sql: refactor pg_builtin to use actual grant options #74172

ecwall · 2021-12-21T21:19:04Z

The builtins has_table_privilege, has_column_privilege,
has_any_column_privilege now use privileges.Priv.GrantOption instead
of privileges.Kind.GRANT.

Refactor builtins.priv -> privilege.Privilege.

Replace privilege.Kind with privilege.Privilege in functions that need
access to privilege.Privilege.GrantOption.

cockroach-teamcity · 2021-12-21T21:19:10Z

This change is

ecwall · 2021-12-21T21:22:44Z

pkg/sql/privilege/privilege.go

+//
+//  res = check(SELECT) || (check(UPDATE) && check(GRANT)) || check(DELETE)
+//
+type Priv struct {


Made this public and moved from builtins to privilege because I thought it made more sense for function signatures that were using privilege.Kind to use privilege.Priv instead of builtins.Priv.

ecwall · 2021-12-21T21:53:52Z

pkg/sql/resolver.go

@@ -314,6 +314,14 @@ func (p *planner) HasPrivilege(
 		return false, err
 	}

+	err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, priv.GrantOption)


Is this correct?

i think instead it should look like:

if priv.GrantOption { err = p.CheckGrantOptionsForUser(..., true /* isGrant */) // rest of the error checking logic }

also, it might need to be done as part of (or more similarly to) hasPrivilegeFunc below.

best way to make sure it's working right is to run the logic tests:

make testbaselogic FILES='privilege_builtins'

(could also be with bazel)

Also, does this need to be wrapped in a version gate to only work in the new version (reverse of this #74172 (comment))?

ecwall · 2021-12-21T21:54:22Z

pkg/sql/sem/builtins/pg_builtins.go

-		if priv.grantOption {
-			// grantOption is set, so AND the result with check(GRANT).
-			d, err = check(privilege.GRANT)
+		if oldVersion { //todo


What is the best way to get this context from here?
p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.DefaultPrivileges)

hm yeah, we don't have a planner here. let me think about it. perhaps the version gate needs to be done deeper in the stack?

rafiss

nit on commit message title -- it shouldn't include the issue number

rafiss · 2021-12-22T18:43:29Z

pkg/sql/sem/builtins/pg_builtins.go

-		if priv.grantOption {
-			// grantOption is set, so AND the result with check(GRANT).
-			d, err = check(privilege.GRANT)
+		if oldVersion { //todo


hm yeah, we don't have a planner here. let me think about it. perhaps the version gate needs to be done deeper in the stack?

rafiss · 2021-12-22T18:46:24Z

pkg/sql/resolver.go

@@ -314,6 +314,14 @@ func (p *planner) HasPrivilege(
 		return false, err
 	}

+	err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, priv.GrantOption)


i think instead it should look like:

if priv.GrantOption { err = p.CheckGrantOptionsForUser(..., true /* isGrant */) // rest of the error checking logic }

also, it might need to be done as part of (or more similarly to) hasPrivilegeFunc below.

best way to make sure it's working right is to run the logic tests:

make testbaselogic FILES='privilege_builtins'

(could also be with bazel)

ecwall · 2021-12-23T21:44:51Z

pkg/sql/sem/builtins/pg_builtins.go

-				"UPDATE WITH GRANT OPTION":     {privilege.UPDATE, true},
-				"REFERENCES":                   {privilege.SELECT, false},
-				"REFERENCES WITH GRANT OPTION": {privilege.SELECT, true},
+				"SELECT":                       {Kind: privilege.SELECT},


These lines were generating errors like this during ./dev build

compilepkg: nogo: errors found by nogo during build-time code analysis: /var/folders/hc/gng26svn1yq13l017705xqjr0000gq/T/rules_go_work-3414600577/github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/pg_builtins.go:559:19: github.com/cockroachdb/cockroach/pkg/sql/privilege.Priv composite literal uses unkeyed fields (composites)

rafiss

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

-- commits, line 4 at r2:
The sentence here would be a good fit for the main commit message body. The release notes are written for users -- so the release note should not include internal implementation details like the names of structs or variables.

pkg/sql/resolver.go, line 306 at r2 (raw file):

	specifier tree.HasPrivilegeSpecifier,
	user security.SQLUsername,
	priv privilege.Priv,

i think this refactor will end up being pretty invasive. it will need a lot of the usages of this function to be updated. that might be OK -- but i think that refactor should be done separately from this PR. so i prefer to keep this as privilege.Kind for now.

pkg/sql/privilege/privilege.go, line 85 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

Made this public and moved from builtins to privilege because I thought it made more sense for function signatures that were using privilege.Kind to use privilege.Priv instead of builtins.Priv.

this is a good point to think about. i feel like as long as this struct is only used within the builtins package, it would be better to keep it there and keep it non-visible

builtins.priv makes sense to me -- it's the representation of a privilege that is relevant for builtin functions. (also the comment on it is very specific for builtin functions too)

pkg/sql/privilege/privilege.go, line 59 at r2 (raw file):

// construction, there is no need for a separate GRANT privilege.
//
// In CockroachDB, there exists a distinct GRANT privilege and no concept of

this comment paragraph and the next one need to be updated. it should mention that we do match the postgres "grant option" behavior

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

Previously, rafiss (Rafi Shamim) wrote…

hm yeah, we don't have a planner here. let me think about it. perhaps the version gate needs to be done deeper in the stack?

i think the best way is to not do the version gate here at all.

instead, we can remove the check for GRANT in this part of the code. then we add the version gate to the hasPrivilege function in pg_builtins.go. on the old version, if priv.GrantOption is true, that function should check for the GRANT privilege, but on the new version it should check for the grant option.

rafiss

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

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

Previously, ecwall (Evan Wall) wrote…

These lines were generating errors like this during ./dev build

compilepkg: nogo: errors found by nogo during build-time code analysis:
/var/folders/hc/gng26svn1yq13l017705xqjr0000gq/T/rules_go_work-3414600577/github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/pg_builtins.go:559:19: github.com/cockroachdb/cockroach/pkg/sql/privilege.Priv composite literal uses unkeyed fields (composites)

yeah, i think since the Priv struct was made public, the linter now requires the field names to be spelled out fully

ecwall

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

-- commits, line 4 at r2:

Previously, rafiss (Rafi Shamim) wrote…

The sentence here would be a good fit for the main commit message body. The release notes are written for users -- so the release note should not include internal implementation details like the names of structs or variables.

Ok, good to know.

pkg/sql/resolver.go, line 306 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this refactor will end up being pretty invasive. it will need a lot of the usages of this function to be updated. that might be OK -- but i think that refactor should be done separately from this PR. so i prefer to keep this as privilege.Kind for now.

The privilege.Kind -> privilege.Priv refactor is required for this check

cockroach/pkg/sql/resolver.go

Line 325 in dd8a8d8

if priv.GrantOption {

(not sure how to link in reviewable).

pkg/sql/privilege/privilege.go, line 85 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this is a good point to think about. i feel like as long as this struct is only used within the builtins package, it would be better to keep it there and keep it non-visible

builtins.priv makes sense to me -- it's the representation of a privilege that is relevant for builtin functions. (also the comment on it is very specific for builtin functions too)

It is passed down and eventually used in resolver.go (

cockroach/pkg/sql/resolver.go

Line 325 in dd8a8d8

if priv.GrantOption {

). Is there a different place I should be checking priv.GrantOption?

rafiss

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

pkg/sql/resolver.go, line 306 at r2 (raw file):

Previously, ecwall (Evan Wall) wrote…

The privilege.Kind -> privilege.Priv refactor is required for this check

cockroach/pkg/sql/resolver.go

Line 325 in dd8a8d8

if priv.GrantOption {

(not sure how to link in reviewable).

i was suggesting to do the check in hasPrivilege in pg_builtins.go , and undo all the changes in resolver.go. this would also allow the priv struct to remain private in the builtins package. but i think what you're doing here is better. i was also wrong about this refactor being invasive; the function is only used in one place. (i remember now that i checked this when writing up the github issue)

pkg/sql/resolver.go, line 326 at r3 (raw file):

		}
		/*if priv.GrantOption { todo
			err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true)

so here we could have:

if priv.GrantOption {
  if !p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption) {
    ctx.Planner.CheckPrivilegeForUser( ... GRANT)
  } else {
    ctx.Planner.CheckGrantOptionsForUser( ... )
 }

(of course, with the correct error handling)

pkg/sql/privilege/privilege.go, line 85 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

It is passed down and eventually used in resolver.go (

cockroach/pkg/sql/resolver.go

Line 325 in dd8a8d8

if priv.GrantOption {

). Is there a different place I should be checking priv.GrantOption?

my suggestion was to call CheckGrantOptions from the HasPrivilege function of builtins.go -- but after thinking a bit more i think that maybe what you're doing now is better

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

Previously, rafiss (Rafi Shamim) wrote…

i think the best way is to not do the version gate here at all.

instead, we can remove the check for GRANT in this part of the code. then we add the version gate to the hasPrivilege function in pg_builtins.go. on the old version, if priv.GrantOption is true, that function should check for the GRANT privilege, but on the new version it should check for the grant option.

my bad; actually let's do the version gate in planner.HasPrivilege. see my comment there.

ecwall

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

pkg/sql/privilege/privilege.go, line 50 at r4 (raw file):

// Privilege represents a privilege parsed from an Access Privilege Inquiry
// Function's privilege string argument.
type Privilege struct {

It seems like postgres doesn't have a term for a container concept like this around the privilege (what we call Kind) and its grant option so I think calling it Privilege is fine unless you have a better name.

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

ecwall

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

a discussion (no related file):
Is this function still needed? Or can it be replaced by calling check inside runPrivilegeChecks?

func runSinglePrivilegeCheck(
	priv privilege.Privilege, check func(privilege.Privilege) (tree.Datum, error),
) (tree.Datum, error) {
	d, err := check(priv)
	if err != nil {
		return nil, err
	}
	switch d {
	case tree.DBoolFalse, tree.DNull:
	case tree.DBoolTrue:
		if priv.GrantOption {
			// GrantOption is set, so AND the result with check(GRANT).
			d, err = check(privilege.Privilege{Kind: privilege.GRANT})
			if err != nil {
				return nil, err
			}
			switch d {
			case tree.DBoolFalse, tree.DBoolTrue, tree.DNull:
			default:
				return nil, errors.AssertionFailedf(
					"unexpected privilege check result %v", d)
			}
		}
	default:
		return nil, errors.AssertionFailedf(
			"unexpected privilege check result %v", d)
	}
	return d, nil
}

rafiss

could you update the PR description so it summarizes the commits? or you could just copy the commit messages into the PR description

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

-- commits, line 2 at r5:
the first commit message title should be updated -- this commit is just a refactor

pkg/sql/resolver.go, line 324 at r6 (raw file):

					err = p.CheckPrivilegeForUser(ctx, desc, privilege.GRANT, user)
				} else {
					err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true)

nit: when passing in constant values to a function, the parameter name should be documented, like this:

p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true /* isGrant */)

pkg/sql/resolver.go, line 329 at r6 (raw file):

		}
		if err != nil {
			if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege {

we'll need to be a bit careful with the error handling here. i believe a different code is returned by CheckGrantOptionsForUser

pkg/sql/privilege/privilege.go, line 50 at r4 (raw file):

Previously, ecwall (Evan Wall) wrote…

It seems like postgres doesn't have a term for a container concept like this around the privilege (what we call Kind) and its grant option so I think calling it Privilege is fine unless you have a better name.

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

sounds good

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

		if priv.GrantOption {
			// GrantOption is set, so AND the result with check(GRANT).
			d, err = check(privilege.Privilege{Kind: privilege.GRANT})

this should be removed in the 2nd commit, since this is now checked in HasPrivilege

ecwall

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

pkg/sql/resolver.go, line 329 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we'll need to be a bit careful with the error handling here. i believe a different code is returned by CheckGrantOptionsForUser

Changed it to WarningPrivilegeNotGranted for that case.

pkg/sql/privilege/privilege.go, line 59 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this comment paragraph and the next one need to be updated. it should mention that we do match the postgres "grant option" behavior

Updated

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

Previously, rafiss (Rafi Shamim) wrote…

this should be removed in the 2nd commit, since this is now checked in HasPrivilege

I added a todo above with details.

ecwall

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

pkg/sql/resolver.go, line 324 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: when passing in constant values to a function, the parameter name should be documented, like this:
p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true /* isGrant */)

Updated

ecwall

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

pkg/sql/resolver.go, line 329 at r6 (raw file):

Previously, ecwall (Evan Wall) wrote…

Changed it to WarningPrivilegeNotGranted for that case.

Actually it looks like that was causing tests to fail so I changed it back.

Refactor builtins.priv -> privilege.Privilege. Replace privilege.Kind with privilege.Privilege in functions that need access to privilege.Privilege.GrantOption. Release note: None

The builtins has_table_privilege, has_column_privilege, has_any_column_privilege now use privileges.Priv.GrantOption instead of privileges.Kind.GRANT. Release note: None

rafiss

lgtm!

Reviewed 1 of 6 files at r4, 1 of 1 files at r5, 1 of 4 files at r7, 1 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @rafiss)

ecwall · 2022-01-12T15:24:56Z

bors r=rafiss

craig · 2022-01-12T16:16:15Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-01-12T18:04:45Z

Build succeeded:

GitHub CI (Cockroach)

ecwall commented Dec 21, 2021

View reviewed changes

ecwall requested review from rafiss and a team December 21, 2021 21:54

rafiss requested changes Dec 22, 2021

View reviewed changes

ecwall commented Dec 23, 2021

View reviewed changes

rafiss requested changes Jan 4, 2022

View reviewed changes

rafiss reviewed Jan 4, 2022

View reviewed changes

ecwall commented Jan 4, 2022

View reviewed changes

rafiss changed the title ~~73129 sql: refactor pg_builtin to use actual grant options~~ sql: refactor pg_builtin to use actual grant options Jan 5, 2022

rafiss requested changes Jan 5, 2022

View reviewed changes

ecwall marked this pull request as ready for review January 6, 2022 16:03

ecwall requested review from a team and stevendanna and removed request for a team January 6, 2022 16:03

ecwall commented Jan 6, 2022

View reviewed changes

rafiss requested review from a team and removed request for a team and stevendanna January 6, 2022 19:19

ecwall commented Jan 6, 2022

View reviewed changes

rafiss requested changes Jan 6, 2022

View reviewed changes

ecwall requested a review from a team January 10, 2022 20:14

ecwall commented Jan 10, 2022

View reviewed changes

ecwall commented Jan 11, 2022

View reviewed changes

ecwall added 2 commits January 12, 2022 10:02

sql: refactor pg_builtin to use actual grant options

70dc9c8

Refactor builtins.priv -> privilege.Privilege. Replace privilege.Kind with privilege.Privilege in functions that need access to privilege.Privilege.GrantOption. Release note: None

sql: refactor pg_builtin to use actual grant options

b43f6bb

The builtins has_table_privilege, has_column_privilege, has_any_column_privilege now use privileges.Priv.GrantOption instead of privileges.Kind.GRANT. Release note: None

rafiss approved these changes Jan 12, 2022

View reviewed changes

craig bot merged commit a4ef738 into cockroachdb:master Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: refactor pg_builtin to use actual grant options #74172

sql: refactor pg_builtin to use actual grant options #74172

ecwall commented Dec 21, 2021 •

edited by rafiss

Loading

cockroach-teamcity commented Dec 21, 2021

ecwall Dec 21, 2021

ecwall Dec 21, 2021

rafiss Dec 22, 2021

ecwall Dec 22, 2021

ecwall Dec 21, 2021

rafiss Dec 22, 2021

rafiss left a comment

rafiss Dec 22, 2021

rafiss Dec 22, 2021

ecwall Dec 23, 2021

rafiss left a comment

rafiss left a comment

ecwall left a comment

rafiss left a comment

ecwall left a comment

ecwall left a comment

rafiss left a comment

ecwall left a comment

ecwall left a comment

ecwall left a comment

rafiss left a comment

ecwall commented Jan 12, 2022

craig bot commented Jan 12, 2022

craig bot commented Jan 12, 2022

sql: refactor pg_builtin to use actual grant options #74172

sql: refactor pg_builtin to use actual grant options #74172

Conversation

ecwall commented Dec 21, 2021 • edited by rafiss Loading

cockroach-teamcity commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

ecwall left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ecwall commented Jan 12, 2022

craig bot commented Jan 12, 2022

craig bot commented Jan 12, 2022

ecwall commented Dec 21, 2021 •

edited by rafiss

Loading