sql: with grant option/grant option for

[jackcwu]

Release note (sql change): If the WITH GRANT OPTION flag is present when granting privileges to a user, then that user is able to grant those same privileges to subsequent users; otherwise, they cannot. If the GRANT OPTION FOR flag is present when revoking privileges from a user, then only the ability the grant those privileges is revoked from that user, not the privileges themselves

[cockroach-teamcity]

This change is 

[jackcwu]

Putting this up for now for preliminary feedback, still have to make a mixed version cluster test and also implement functionality for default privileges

[RichardJCai]

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

---

pkg/sql/catalog/descpb/privilege.go, line 192 at r1 (raw file):

// could be different from the privileges. Unlike the normal grant, it does not bind
// the grant options to the privileges.
func (p *PrivilegeDescriptor) GranularGrant(

Just to clarify there can be a case where we're GRANTing just the with grant option and not the actual privilege itself right?

Also I'd add some unit tests to cover these cases inprivilege_test.go which currently just tests Grant.

Otherwise I think the logic here looks solid.

[RichardJCai]

Looking good so far, just leaving some notes

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

---

pkg/sql/catalog/descpb/privilege.go, line 177 at r2 (raw file):

	userPriv, _ := p.FindUser(user)

	// User can grant anything; check passes

nit: update comment to user has ALL WITH GRANT OPTION so they can grant anything.

---

pkg/sql/catalog/descpb/privilege.go, line 194 at r2 (raw file):

// GranularGrant adds new privileges to this descriptor and new grant options which
// could be different from the privileges. Unlike the normal grant, it does not bind
// the grant options to the privileges.

I'm a bit confused about this description. What does bind the grant option to the privileges means?
Also can you explain more about the difference between GranularGrant and Grant and when each one is used?

Why do we only need to do this for Grant and not Revoke?

---

pkg/sql/catalog/descpb/privilege.go, line 244 at r2 (raw file):

	}

	// Consider the case where it has all privileges but with grant option - don't want to

nit: would change this comment to just say "we only have to ..."

---

pkg/sql/catalog/descpb/privilege.go, line 316 at r2 (raw file):

	// One doesn't see "AND NOT" very often.
	userPriv.WithGrantOption &^= bits

Should this be run even if grantOptionFor is false?

---

pkg/sql/parser/sql.y, line 4346 at r2 (raw file):

// %SeeAlso: GRANT, WEBDOCS/revoke.html
revoke_stmt:
	REVOKE privileges ON targets FROM role_spec_list

nit: spaces instead of tabs here for consistency

---

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

	charToGrantOption := map[string]bool{} // tracks if one has grant option on char/privilege
	for _, privilege := range privileges {
		//switch privilege {

Nice refactor, let's delete the switch statement

---

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

		starredChars = append(starredChars, aclChar)
		if _, ok := charToGrantOption[aclChar]; ok {
			starredChars = append(starredChars, "*")

Can we do this in one pass with the privileges and the grant option?

[jackcwu]

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

---

pkg/sql/catalog/descpb/privilege.go, line 192 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Just to clarify there can be a case where we're GRANTing just the with grant option and not the actual privilege itself right?

Also I'd add some unit tests to cover these cases inprivilege_test.go which currently just tests Grant.

Otherwise I think the logic here looks solid.

Yep! For instance, a user has all privileges but no grant options. If we were to grant SELECT WITH GRANT OPTION then the SELECT wouldn't change anything to the privileges since the user has ALL but the SELECT would be applied to the grant option bits

---

pkg/sql/catalog/descpb/privilege.go, line 177 at r2 (raw file):

user has ALL WITH GRANT OPTION so they can grant anything.
fixed

---

pkg/sql/catalog/descpb/privilege.go, line 194 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I'm a bit confused about this description. What does bind the grant option to the privileges means?
Also can you explain more about the difference between GranularGrant and Grant and when each one is used?

Why do we only need to do this for Grant and not Revoke?

Yes, so the way I thought of this was basically centralized around how privileges are aggregated on descriptors. With a normal grant statement, grant options are an all-or-nothing deal for the grant bits - if you include the flag, then every privilege in that statement also gets grant options, or no grant options are given if the flag is omitted (this is what I mean by "bind the grant options to the privileges")

So when an object (tables, schema, etc) is created, it makes a blank descriptor with just root and admin on it, then iterates through all of the existing descriptors to add to itself to make the privileges. As a user, you can continuously modify the those individual descriptors with regular grant and revoke statements to create various combinations of privileges and grant bits.

Basically granular grant is a shortcut that applies the state of the descriptor onto the new descriptor for the object being created. The regular grant wouldn't cut it because what if Alice's ALTER DEFAULT PRIVILEGES state on tables is ALL privileges but grant option on SELECT and INSERT? A normal grant could not recreate that since if you granted ALL privileges you would either grant ALL grant options or no grant options.

The reason there isn't a granular revoke is because this logic is only triggered when we create an object (which by default is a blank slate) and have to give it the default privileges through the descriptors. If we revoked everything from a descriptor previously, then applying it to the descriptor created by this object's creation simply wouldn't do anything

---

pkg/sql/catalog/descpb/privilege.go, line 244 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: would change this comment to just say "we only have to ..."

fixed

---

pkg/sql/catalog/descpb/privilege.go, line 316 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Should this be run even if grantOptionFor is false?

yes; if the "GRANT OPTION FOR" flag is not specified, then according the REVOKE documentation both the privileges and the grant options are revoked. So therefore no matter what, the grant option bits are being affected. If not, then we might end up with a case where a user does not have the privileges but does have the grant option

---

pkg/sql/parser/sql.y, line 4346 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: spaces instead of tabs here for consistency

fixed

---

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

Previously, RichardJCai (Richard Cai) wrote…

Nice refactor, let's delete the switch statement

fixed

---

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

Previously, RichardJCai (Richard Cai) wrote…

Can we do this in one pass with the privileges and the grant option?

fixed

[RichardJCai]

Reviewed 9 of 18 files at r1, 9 of 13 files at r2, 5 of 6 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @rafiss, @RichardJCai, and @stevendanna)

---

pkg/cmd/roachtest/cluster.go, line 2319 at r3 (raw file):

	//dataSourceName := fmt.Sprintf("%s?user=%s?password=%s", urls[0], user, password)
	dataSourceName := strings.Replace(urls[0], "root", user, 1)
	fmt.Println("datasourcename is: ", dataSourceName)

Remove print and commented out code here.

Also I would change this function to be called ConnEAsUser

Edit: actually it looks like we don't actually use this function in this existing PR, let's only add this function in the test PR where it's actually used.

---

pkg/sql/grant_revoke.go, line 184 at r3 (raw file):

			privileges := descriptor.GetPrivileges()

			// Ensure that the granter is allowed to grant the privileges

nit: I don't think this comment is actually necessary but if so, please expand in the comment that we only check the grant option on the given version. Also end comment in period.

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 194 at r3 (raw file):

	//   Issue #67378.
	if targetObject == tree.Tables || targetObject == tree.Sequences {
		//fmt.Println("tables or sequences loop")

Remove prints here

---

pkg/sql/catalog/descpb/privilege.go, line 194 at r2 (raw file):

Previously, jackcwu wrote…

Yes, so the way I thought of this was basically centralized around how privileges are aggregated on descriptors. With a normal grant statement, grant options are an all-or-nothing deal for the grant bits - if you include the flag, then every privilege in that statement also gets grant options, or no grant options are given if the flag is omitted (this is what I mean by "bind the grant options to the privileges")

So when an object (tables, schema, etc) is created, it makes a blank descriptor with just root and admin on it, then iterates through all of the existing descriptors to add to itself to make the privileges. As a user, you can continuously modify the those individual descriptors with regular grant and revoke statements to create various combinations of privileges and grant bits.

Basically granular grant is a shortcut that applies the state of the descriptor onto the new descriptor for the object being created. The regular grant wouldn't cut it because what if Alice's ALTER DEFAULT PRIVILEGES state on tables is ALL privileges but grant option on SELECT and INSERT? A normal grant could not recreate that since if you granted ALL privileges you would either grant ALL grant options or no grant options.

The reason there isn't a granular revoke is because this logic is only triggered when we create an object (which by default is a blank slate) and have to give it the default privileges through the descriptors. If we revoked everything from a descriptor previously, then applying it to the descriptor created by this object's creation simply wouldn't do anything

I see - that makes sense, however would it be possible to somehow bake GranularGrant into the init of the descriptor instead.

Ie, in newPrivs := descpb.NewDefaultPrivilegeDescriptor(user) we pass what we need to do the GranularGrant and make GranularGrant a private method instead.

We shouldn't expose generally if it should only be on init imo.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_with_grant_option, line 49 at r3 (raw file):

statement error pq: missing WITH GRANT OPTION privilege type GRANT
GRANT GRANT, SELECT ON TABLE t1 to target

This is a bit strange to me, I think we should either deprecate GRANT or continue to check GRANT in addition to GRANT OPTION in the current version.
I think I'd prefer to explore if we can deprecate GRANT in this verison. Doesn't have to be in this PR but please open a followup issue for this.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_with_grant_option, line 82 at r3 (raw file):

test           public       t2          testuser  SELECT

statement error pq: missing WITH GRANT OPTION privilege type GRANT

We should also expand SHOW GRANTS to support showing WITH GRANT OPTIONs as well to make this easier to test.

---

pkg/sql/privilege/privilege.go, line 289 at r3 (raw file):

	for _, privilege := range orderedPrivs {
		if _, ok := privToACL[privilege]; !ok {
			continue

I think we should actually raise an assertion error here

[jackcwu]

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

---

pkg/sql/grant_revoke.go, line 184 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: I don't think this comment is actually necessary but if so, please expand in the comment that we only check the grant option on the given version. Also end comment in period.

removed

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 194 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove prints here

removed

---

pkg/sql/catalog/descpb/privilege.go, line 194 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I see - that makes sense, however would it be possible to somehow bake GranularGrant into the init of the descriptor instead.

Ie, in newPrivs := descpb.NewDefaultPrivilegeDescriptor(user) we pass what we need to do the GranularGrant and make GranularGrant a private method instead.

We shouldn't expose generally if it should only be on init imo.

Good point. I don't think GranularGrant makes sense where it is currently since it does in fact only happen in the default case on init. I'm also not a big fan of moving the logic in CreatePrivilegesFromDefaultPrivileges from defaultprivilege.go into a separate function in descpb.privilege.go since I think that logic should stay in the original file.

My solution was to remove the *privilegeDescriptor from the receiver of GranularGrant and add it as a parameter instead; then take that whole function and move it into defaultprivilege.go, renaming it as granularGrant so that it is private and exposed only to the work that happens in regards to default privileges.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_with_grant_option, line 49 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

This is a bit strange to me, I think we should either deprecate GRANT or continue to check GRANT in addition to GRANT OPTION in the current version.
I think I'd prefer to explore if we can deprecate GRANT in this verison. Doesn't have to be in this PR but please open a followup issue for this.

Opened a followup issue. The thing I was testing here was if individual privileges get stopped when trying to grant without grant options. Unfortunately this does mean I have to grant GRANT as well for the time being since if I don't, then the prior validation check on whether a user holds the privilege itself stops the statement from executing.

Depending on when the deprecation happens, this does indeed mean that these test cases will have to be changed again afterwards

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_with_grant_option, line 82 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

We should also expand SHOW GRANTS to support showing WITH GRANT OPTIONs as well to make this easier to test.

Yep, this makes sense - SHOW GRANTS as we use it in cockroachDB doesn't seem to have a precedent in postgres. Should I do something like adding a * after the privilege if it has grant options? OR perhaps another column with heading 'Grant Option' and whether the grant option is there for that privilege.

There is an edge case where a user has ALL PRIVILEGES on something but only certain grant options (ex. having ALL PRIVILEGES on a table but only SELECT and INSERT grant options). In this case, it might be good to have a separate column that lists the grant option privileges for each privilege. This may be a good question for Vy

---

pkg/sql/privilege/privilege.go, line 289 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we should actually raise an assertion error here

added

---

pkg/cmd/roachtest/cluster.go, line 2319 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove print and commented out code here.

Also I would change this function to be called ConnEAsUser

Edit: actually it looks like we don't actually use this function in this existing PR, let's only add this function in the test PR where it's actually used.

removed the entire function, it's in my following PR with the roachtest/migration

[RichardJCai]

Reviewed 12 of 21 files at r4, 7 of 8 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @rafiss, @RichardJCai, and @stevendanna)

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 324 at r6 (raw file):

			UserProto:       security.PublicRoleName().EncodeProto(),
			Privileges:      privilege.USAGE.Mask(),
			WithGrantOption: privilege.USAGE.Mask(),

I think WithGrantOption here should actually be 0, this case refers to the Default case where the public role has USAGE on types.
However I think we checked that the default case is public has USAGE but not with grant option right?

I'm pretty sure this is the case otherwise anyone would be able to grant USAGE on types in PG by default since everyone is a member of the public role in PG.

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 338 at r6 (raw file):

			UserProto:       userProto,
			Privileges:      privilege.ALL.Mask(),
			WithGrantOption: privilege.ALL.Mask(),

Ditto with the comment above, WithGrantOption should be 0 I believe.

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 661 at r6 (raw file):

		grantGrantOptions   privilege.List
		expectedPrivileges  privilege.List
		expectedGrantOption privilege.List

I think we should maybe add a test case where we have a privilege in grantGrantOptions but not in grantPrivileges and verify that it shouldn't be possible. (Possibly return an error there?)

---

pkg/sql/catalog/descpb/privilege.go, line 316 at r2 (raw file):

Previously, jackcwu wrote…

yes; if the "GRANT OPTION FOR" flag is not specified, then according the REVOKE documentation both the privileges and the grant options are revoked. So therefore no matter what, the grant option bits are being affected. If not, then we might end up with a case where a user does not have the privileges but does have the grant option

Understood

---

pkg/sql/catalog/descpb/privilege.go, line 227 at r6 (raw file):

	}

	// We only have to modify grant option.

I think this comment is slightly out of place, should it be inside the if statement below?

---

pkg/sql/catalog/descpb/privilege.go, line 285 at r6 (raw file):

	}

	if privilege.ALL.IsSetIn(userPriv.WithGrantOption) {

So the reason we don't need a &&grantOptionFor here is because we always revoke the GRANT OPTION if we're revoking hte privilege right?

Can we add a comment about that to make it clear.

---

pkg/sql/catalog/descpb/privilege_test.go, line 565 at r6 (raw file):

// TestRevokeWithGrantOption tests whether revoking grant option for changes the
// privilege bits and grant option bits as expected.
func TestRevokeWithGrantOption(t *testing.T) {

Are there tests that verify if we have GRANT OPTION and PRIVILEGES doing a regular revoke also takes away the GRANT OPTIONS?

---

pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 62 at r6 (raw file):

user testuser2

statement error pq: missing WITH GRANT OPTION privilege type SELECT

I think we should update this error to say missing WITH GRANT OPTION on privilege type SELECT, actually could you double check what the PG error is and copy that?

---

pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 297 at r6 (raw file):

GRANT CONNECT ON DATABASE d TO target WITH GRANT OPTION

# ---------------------------OWNER OF AN OBJECT WILL ALWAYS BE ABLE TO GRANT/REVOKE--------------------------------------------------------

nit: remove the ---------------

[RichardJCai]

Ah one more thing, I'd like to see some tests for this where we're granting WITH GRANT OPTION as well

[RichardJCai]

nit: remove comment

[RichardJCai]

nit: remove comment

[rafiss]

awesome! my comments all have to do with style and asking for some clarification for my own understanding. nice work on this

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

---

pkg/sql/grant_revoke.go, line 184 at r6 (raw file):

			privileges := descriptor.GetPrivileges()

			if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption) {

maybe a nit, not sure if it matters, but should the clusterversion where we start checking grant option be after the one where we do the long-running migration?

also FYI I edited the description of #73065 so that as a follow-up, we remove the old logic to check the GRANT privilege.

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 74 at r6 (raw file):

	grantees []security.SQLUsername,
	targetObject tree.AlterDefaultPrivilegesTargetObject,
	withGrantOption bool,

to avoid having to do all those inline comments, you could also add something like

type WithGrantOptionType bool

const (
	WithGrantOption WithGrantOptionType = true
	WithoutGrantOption WithGrantOptionType = false
)

see an example:

cockroach/pkg/security/username.go

Line 145 in 6d024a9

type UsernamePurpose bool

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 298 at r6 (raw file):

) {
	if targetObject == tree.Types && GetPublicHasUsageOnTypes(defaultPrivilegesForRole) {
		privileges.Grant(security.PublicRoleName(), privilege.List{privilege.USAGE}, false)

nit: for raw constants, the convention is to add an inline comment. so:

privileges.Grant(security.PublicRoleName(), privilege.List{privilege.USAGE}, false /* withGrantOption */)

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 306 at r6 (raw file):

	}
	if GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, targetObject) {
		privileges.Grant(defaultPrivilegesForRole.GetExplicitRole().UserProto.Decode(), privilege.List{privilege.ALL}, false)

nit:

..., false /* withGrantOption */)

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 324 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think WithGrantOption here should actually be 0, this case refers to the Default case where the public role has USAGE on types.
However I think we checked that the default case is public has USAGE but not with grant option right?

I'm pretty sure this is the case otherwise anyone would be able to grant USAGE on types in PG by default since everyone is a member of the public role in PG.

nice catch - let's make sure there's a test

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 390 at r6 (raw file):

// could be different from the privileges. Unlike the normal grant, the privileges
// and the grant options being granted could be different
func granularGrant(

is default_privilege.go the right location for this function?

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 399 at r6 (raw file):

	if privilege.ALL.IsSetIn(userPriv.WithGrantOption) {
		// User already has 'ALL' privilege: no-op.
		// If userPriv.WithGrantOption has ALL, then userPriv.Privileges must also have ALL.

although this is true, i think it would be good to be a bit defensive here and also check privilege.ALL.IsSetIn(userPriv.Privileges

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 167 at r6 (raw file):

		defaultPrivileges := NewMutableDefaultPrivileges(defaultPrivilegeDescriptor)

		defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.privileges, tc.grantees, tc.targetObject, false)

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 285 at r6 (raw file):

		defaultPrivileges := NewMutableDefaultPrivileges(defaultPrivilegeDescriptor)

		defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.grantPrivileges, tc.grantees, tc.targetObject, false)

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 311 at r6 (raw file):

	defaultPrivileges.RevokeDefaultPrivileges(descpb.DefaultPrivilegesRole{
		Role: creatorUser,
	}, privilege.List{privilege.ALL}, []security.SQLUsername{fooUser}, tree.Tables, false)

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 537 at r6 (raw file):

				userAndGrant.grants,
				[]security.SQLUsername{userAndGrant.user},
				tc.targetObject, false,

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 598 at r6 (raw file):

			tc.revokeAndGrantPrivileges,
			[]security.SQLUsername{creatorUser},
			tc.targetObject, false,

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 607 at r6 (raw file):

			tc.revokeAndGrantPrivileges,
			[]security.SQLUsername{creatorUser},
			tc.targetObject, false,

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 631 at r6 (raw file):

		privilege.List{privilege.USAGE},
		[]security.SQLUsername{security.PublicRoleName()},
		tree.Types, false,

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 640 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ah one more thing, I'd like to see some tests for this where we're granting WITH GRANT OPTION as well

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/fix_test.go, line 144 at r6 (raw file):

		desc := &descpb.PrivilegeDescriptor{}
		for u, p := range testCase.input {
			desc.Grant(u, p, false)

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/fix_test.go, line 378 at r6 (raw file):

		desc := &descpb.PrivilegeDescriptor{Version: tc.privDescVersion}
		for u, p := range tc.input {
			desc.Grant(u, p, false)

nit: add inline comment for false

---

pkg/sql/catalog/catprivilege/fix_test.go, line 485 at r6 (raw file):

		desc := &descpb.PrivilegeDescriptor{}
		for u, p := range tc.input {
			desc.Grant(u, p, false)

nit: add inline comment for false

---

pkg/sql/catalog/descpb/privilege.go, line 215 at r6 (raw file):

	if privilege.ALL.IsSetIn(userPriv.WithGrantOption) {
		// User already has 'ALL' privilege: no-op.
		// If userPriv.WithGrantOption has ALL, then userPriv.Privileges must also have ALL.

although this is true, i think it would be good to be a bit defensive here and also check privilege.ALL.IsSetIn(userPriv.Privileges

also, isn't this logic repeated in granularGrant?

---

pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 62 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we should update this error to say missing WITH GRANT OPTION on privilege type SELECT, actually could you double check what the PG error is and copy that?

weirdly, postgres does not error, but it does show a warning

postgres=> grant select on t to new_user;
WARNING:  no privileges were granted for "t"
GRANT

i think we should keep this behavior where we error, otherwise it's a more backward incompatible change

[jackcwu]

Thank you! Just addressed the comments

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

---

pkg/sql/grant_revoke.go, line 184 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

maybe a nit, not sure if it matters, but should the clusterversion where we start checking grant option be after the one where we do the long-running migration?

also FYI I edited the description of #73065 so that as a follow-up, we remove the old logic to check the GRANT privilege.

please correct me if I'm wrong, but since this version gate ensures that ALL of the nodes in the cluster must be at least the specified version for it to run, I think this would still work.

Since even though the migration upgrades nodes to that same version, it won't run prematurely since everything must be upgraded for it to work (which implies that the migration has finished)

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 74 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

to avoid having to do all those inline comments, you could also add something like

type WithGrantOptionType bool

const (
	WithGrantOption WithGrantOptionType = true
	WithoutGrantOption WithGrantOptionType = false
)

see an example:

cockroach/pkg/security/username.go

Line 145 in 6d024a9

type UsernamePurpose bool

makes sense! Since there weren't that many instances I decided to just add inlines comments to each time I put in a raw constant

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 298 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: for raw constants, the convention is to add an inline comment. so:

privileges.Grant(security.PublicRoleName(), privilege.List{privilege.USAGE}, false /* withGrantOption */)

added

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 306 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit:

..., false /* withGrantOption */)

added

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 324 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nice catch - let's make sure there's a test

added a test in grant_revoke_with_grant_option

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 338 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ditto with the comment above, WithGrantOption should be 0 I believe.

removed. I originally did this to make the uninhibited behavior with object owners work, but i will bake that into the validation check instead

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 390 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is default_privilege.go the right location for this function?

this one is a bit interesting, since it could potentially go here or in descpb/privilege.go. I ultimately put this in default_privilege.go because it is only called one time ever - when applying default privileges to an object upon its creation, and made it private as to prevent its exposure to other situations

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 399 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

although this is true, i think it would be good to be a bit defensive here and also check privilege.ALL.IsSetIn(userPriv.Privileges

changed the condition to be if privilege.ALL.IsSetIn(userPriv.WithGrantOption) && privilege.ALL.IsSetIn(userPriv.Privileges)

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 167 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 285 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 311 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 537 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 598 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 607 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 631 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 661 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we should maybe add a test case where we have a privilege in grantGrantOptions but not in grantPrivileges and verify that it shouldn't be possible. (Possibly return an error there?)

makes sense, but this is a bit tricky to test since I would have to do this in the logic of granularGrant for it to mean anything.

My solution was to add some logic to iterate through the grant option privileges in the function granularGrant and basically exit if there was a privilege that was granted in the grant options but not the privileges (so nothing gets applied)

We can check this in the test case by having the expected be the same as the original inputs - we know that a privilege in grantGrantOptions but not in grantPrivileges is rejected this way

---

pkg/sql/catalog/catprivilege/fix_test.go, line 144 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/fix_test.go, line 378 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/catprivilege/fix_test.go, line 485 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added

---

pkg/sql/catalog/descpb/privilege.go, line 316 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Understood

.

---

pkg/sql/catalog/descpb/privilege.go, line 118 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: remove comment

removed

---

pkg/sql/catalog/descpb/privilege.go, line 123 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: remove comment

removed

---

pkg/sql/catalog/descpb/privilege.go, line 215 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

although this is true, i think it would be good to be a bit defensive here and also check privilege.ALL.IsSetIn(userPriv.Privileges

also, isn't this logic repeated in granularGrant?

changed the condition to if privilege.ALL.IsSetIn(userPriv.WithGrantOption) && privilege.ALL.IsSetIn(userPriv.Privileges)

granularGrant is a pretty similar function but it is different in that it only acts on applying default privileges to an object upon creation, where it is possible to have different privileges and grant options (whereas on a normal grant, you either get grant options for all the privileges you grant, or none). Pasting a prior comment with a more in-depth explanation below:

Yes, so the way I thought of this was basically centralized around how privileges are aggregated on descriptors. With a normal grant statement, grant options are an all-or-nothing deal for the grant bits - if you include the flag, then every privilege in that statement also gets grant options, or no grant options are given if the flag is omitted (this is what I mean by "bind the grant options to the privileges")

So when an object (tables, schema, etc) is created, it makes a blank descriptor with just root and admin on it, then iterates through all of the existing descriptors to add to itself to make the privileges. As a user, you can continuously modify the those individual descriptors with regular grant and revoke statements to create various combinations of privileges and grant bits.

Basically granular grant is a shortcut that applies the state of the descriptor onto the new descriptor for the object being created. The regular grant wouldn't cut it because what if Alice's ALTER DEFAULT PRIVILEGES state on tables is ALL privileges but grant option on SELECT and INSERT? A normal grant could not recreate that since if you granted ALL privileges you would either grant ALL grant options or no grant options.

The reason there isn't a granular revoke is because this logic is only triggered when we create an object (which by default is a blank slate) and have to give it the default privileges through the descriptors. If we revoked everything from a descriptor previously, then applying it to the descriptor created by this object's creation simply wouldn't do anything

---

pkg/sql/catalog/descpb/privilege.go, line 227 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think this comment is slightly out of place, should it be inside the if statement below?

removed it, seems unnecessary

---

pkg/sql/catalog/descpb/privilege.go, line 285 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

So the reason we don't need a &&grantOptionFor here is because we always revoke the GRANT OPTION if we're revoking hte privilege right?

Can we add a comment about that to make it clear.

yep exactly. No matter what, the grant option will be modified - if the flag is false, then the privileges will also be modified

comment added

---

pkg/sql/catalog/descpb/privilege_test.go, line 565 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Are there tests that verify if we have GRANT OPTION and PRIVILEGES doing a regular revoke also takes away the GRANT OPTIONS?

there are in the logic tests - if the privileges are revoked, then the statement will be stopped by the validation check on not having the privileges before it ever gets to that grant option check.

Good point though, added some more here

---

pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 62 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we should update this error to say missing WITH GRANT OPTION on privilege type SELECT, actually could you double check what the PG error is and copy that?

changed the error codes to WarningPrivilegeNotGranted and WarningPrivilegeNotRevoked, otherwise keeping the behavior consistent with the privilege check

---

pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 297 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: remove the ---------------

removed

[jackcwu]

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

---

pkg/sql/catalog/catprivilege/default_privilege_test.go, line 640 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add inline comment for false

added inline comment

I added some tests here and made some changes to the public/type case that correspond with the generic cases previously. Basically, I would only remove the user from the privilege descriptor and set the flag to true if there are no changes to it, since the flag acts as a way to "store" the privileges. Having grant options counts as a modification and we need to look at the privilege descriptor for that, so in that case we do not remove the user (added a check for USAGE in the grant options)

[RichardJCai]

Awesome! LGTM now besides the remaining comments. (Also some logic tests to update again)

Reviewed 3 of 21 files at r4, 1 of 2 files at r5, 7 of 9 files at r7, 9 of 9 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @rafiss, @RichardJCai, and @stevendanna)

---

-- commits, line 4 at r8:
I would add some more detail on how the syntax is actually different (give some examples), also note that this is the same as PG

---

pkg/sql/authorization.go, line 162 at r8 (raw file):

// CheckGrantOption calls ValidateGrantPrivileges(), which will return an error if a user
// tries to grant a privilege it does not have grant options for

nit: period at end of comment

---

pkg/sql/authorization.go, line 167 at r8 (raw file):

) error {
	// Always allow the command to go through if performed by a superuser or the owner of the object
	if p.User().IsRootUser() || p.User().IsAdminRole() || IsOwner(descriptor, p.User()) {

I think UserHasAdminRole should cover the first two cases?

---

pkg/sql/grant_revoke.go, line 184 at r6 (raw file):

Previously, jackcwu wrote…

please correct me if I'm wrong, but since this version gate ensures that ALL of the nodes in the cluster must be at least the specified version for it to run, I think this would still work.

Since even though the migration upgrades nodes to that same version, it won't run prematurely since everything must be upgraded for it to work (which implies that the migration has finished)

@jackcwu do you mean that we'll run the migration on the ValidateGrantOption clusterversion? I think that would make sense and that this would make sense with it.

---

pkg/sql/grant_revoke.go, line 185 at r8 (raw file):

			if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption) {
				err := p.CheckGrantOption(descriptor, n.desiredprivs, n.isGrant) //privileges.ValidateGrantPrivileges(p.User(), n.desiredprivs, n.isGrant)

Remove comment

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 390 at r6 (raw file):

Previously, jackcwu wrote…

this one is a bit interesting, since it could potentially go here or in descpb/privilege.go. I ultimately put this in default_privilege.go because it is only called one time ever - when applying default privileges to an object upon its creation, and made it private as to prevent its exposure to other situations

Maybe we could rename this to applyDefaultPrivileges?

[jackcwu]

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

---

-- commits, line 4 at r8:

Previously, RichardJCai (Richard Cai) wrote…

I would add some more detail on how the syntax is actually different (give some examples), also note that this is the same as PG

added

---

pkg/sql/authorization.go, line 162 at r8 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: period at end of comment

added

---

pkg/sql/authorization.go, line 167 at r8 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think UserHasAdminRole should cover the first two cases?

fixed

---

pkg/sql/grant_revoke.go, line 184 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

@jackcwu do you mean that we'll run the migration on the ValidateGrantOption clusterversion? I think that would make sense and that this would make sense with it.

yep! So I would define the ValidateGrantOption cluster version and use it both for this version gate and the migration on my other PR

---

pkg/sql/grant_revoke.go, line 185 at r8 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove comment

removed

---

pkg/sql/catalog/catprivilege/default_privilege.go, line 390 at r6 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Maybe we could rename this to applyDefaultPrivileges?

renamed

[jackcwu]

Thanks, just pushed up new changes based on the remaining comments

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

[rafiss]

this lgtm! exciting

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

---

-- commits, line 4 at r10:
nit: make sure to add hard line-wraps in commit messages (should be 80 characters)

---

pkg/sql/grant_revoke.go, line 184 at r6 (raw file):

Previously, jackcwu wrote…

yep! So I would define the ValidateGrantOption cluster version and use it both for this version gate and the migration on my other PR

this sounds good!

[jackcwu]

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

---

-- commits, line 4 at r10:

Previously, rafiss (Rafi Shamim) wrote…

nit: make sure to add hard line-wraps in commit messages (should be 80 characters)

fixed

[jackcwu]

bors r+

[jackcwu]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)