Merge #75560

75560: sql: allow non-ALL privileges to be granted WITH GRANT OPTION r=rafiss a=ecwall Co-authored-by: Evan Wall <[email protected]>
cockroachdb · Feb 9, 2022 · 68d50bc · 68d50bc
2 parents fbccaf9 + 46add16
commit 68d50bc
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 13 deletions.
diff --git a/pkg/sql/catalog/descpb/privilege.go b/pkg/sql/catalog/descpb/privilege.go
@@ -266,9 +266,7 @@ func (p *PrivilegeDescriptor) Grant(
 	if withGrantOption {
 		userPriv.WithGrantOption |= bits
 	}
-	if !privilege.ALL.IsSetIn(userPriv.Privileges) {
-		userPriv.Privileges |= bits
-	}
+	userPriv.Privileges |= bits
 }
 
 // Revoke removes privileges from this descriptor for a given list of users.
@@ -292,6 +290,9 @@ func (p *PrivilegeDescriptor) Revoke(
 			// TODO(marc): the grammar does not allow it, but we should
 			// check if other privileges are being specified and error out.
 			p.RemoveUser(user)
+		} else if privilege.ALL.IsSetIn(userPriv.Privileges) {
+			// fold sub-privileges into ALL
+			userPriv.Privileges = privilege.ALL.Mask()
 		}
 		return
 	}
@@ -331,7 +332,6 @@ func (p *PrivilegeDescriptor) Revoke(
 			p.RemoveUser(user)
 		}
 	}
-
 }
 
 // GrantPrivilegeToGrantOptions adjusts a user's grant option bits based on whether the GRANT or ALL

diff --git a/pkg/sql/catalog/descpb/privilege_test.go b/pkg/sql/catalog/descpb/privilege_test.go
@@ -131,9 +131,9 @@ func TestPrivilege(t *testing.T) {
 		// Ensure revoking CREATE, DROP, GRANT, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG
 		// from a user with ALL privilege on a table leaves the user with no privileges.
 		{testUser,
-			privilege.List{privilege.ALL}, privilege.List{privilege.CREATE, privilege.DROP,
-				privilege.GRANT, privilege.SELECT, privilege.INSERT, privilege.DELETE, privilege.UPDATE,
-				privilege.ZONECONFIG},
+			privilege.List{privilege.ALL},
+			privilege.List{privilege.CREATE, privilege.DROP, privilege.GRANT, privilege.SELECT, privilege.INSERT,
+				privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG},
 			[]descpb.UserPrivilege{
 				{security.AdminRoleName(), []privilege.Privilege{{Kind: privilege.ALL, GrantOption: true}}},
 			},
@@ -142,9 +142,9 @@ func TestPrivilege(t *testing.T) {
 		// Ensure revoking CONNECT, CREATE, DROP, GRANT, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG
 		// from a user with ALL privilege on a database leaves the user with no privileges.
 		{testUser,
-			privilege.List{privilege.ALL}, privilege.List{privilege.CONNECT, privilege.CREATE,
-				privilege.DROP, privilege.GRANT, privilege.SELECT, privilege.INSERT, privilege.DELETE,
-				privilege.UPDATE, privilege.ZONECONFIG},
+			privilege.List{privilege.ALL},
+			privilege.List{privilege.CONNECT, privilege.CREATE, privilege.DROP, privilege.GRANT, privilege.SELECT,
+				privilege.INSERT, privilege.DELETE, privilege.UPDATE, privilege.ZONECONFIG},
 			[]descpb.UserPrivilege{
 				{security.AdminRoleName(), []privilege.Privilege{{Kind: privilege.ALL, GrantOption: true}}},
 			},
@@ -539,7 +539,7 @@ func TestGrantWithGrantOption(t *testing.T) {
 		{descpb.NewPrivilegeDescriptor(testUser, privilege.List{privilege.ALL}, privilege.List{}, security.AdminRoleName()),
 			testUser, privilege.Table,
 			privilege.List{privilege.SELECT, privilege.INSERT},
-			privilege.List{privilege.ALL},
+			privilege.List{privilege.ALL, privilege.SELECT, privilege.INSERT},
 			privilege.List{privilege.SELECT, privilege.INSERT}},
 		{descpb.NewPrivilegeDescriptor(testUser, privilege.List{privilege.INSERT}, privilege.List{}, security.AdminRoleName()),
 			testUser, privilege.Table,

diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go
@@ -219,10 +219,10 @@ func (n *changePrivilegesNode) startExec(params runParams) error {
 				if granteeHasGrantPriv && n.isGrant && !n.withGrantOption && len(noticeMessage) == 0 {
 					noticeMessage = "grant options were automatically applied but this behavior is deprecated"
 				}
-				if grantPresent || allPresent || (granteeHasGrantPriv && n.isGrant) {
+				if !n.withGrantOption && (grantPresent || allPresent || (granteeHasGrantPriv && n.isGrant)) {
 					if n.isGrant {
 						privileges.GrantPrivilegeToGrantOptions(grantee, true /*isGrant*/)
-					} else if !n.isGrant && !n.withGrantOption {
+					} else {
 						privileges.GrantPrivilegeToGrantOptions(grantee, false /*isGrant*/)
 					}
 				}

diff --git a/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option b/pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option
@@ -676,3 +676,57 @@ test           public       t1          admin      ALL             true
 test           public       t1          root       ALL             true
 test           public       t1          testuser   ALL             true
 test           public       t1          testuser2  ALL             true
+
+# non-ALL privileges should appear if ALL does not have grant option, but another privilege does
+user root
+
+statement ok
+CREATE TABLE grant_ordering_table (id INT PRIMARY KEY);
+CREATE USER grant_ordering_user
+
+statement ok
+GRANT ALL ON TABLE grant_ordering_table TO grant_ordering_user
+
+query TTTTTB colnames
+SHOW GRANTS ON grant_ordering_table FOR grant_ordering_user
+----
+database_name  schema_name  table_name            grantee              privilege_type  is_grantable
+test           public       grant_ordering_table  grant_ordering_user  ALL             true
+
+statement ok
+REVOKE GRANT OPTION FOR ALL ON TABLE grant_ordering_table FROM grant_ordering_user
+
+query TTTTTB colnames
+SHOW GRANTS ON grant_ordering_table FOR grant_ordering_user
+----
+database_name  schema_name  table_name            grantee              privilege_type  is_grantable
+test           public       grant_ordering_table  grant_ordering_user  ALL             false
+
+statement ok
+GRANT SELECT ON TABLE grant_ordering_table TO grant_ordering_user WITH GRANT OPTION
+
+query TTTTTB colnames
+SHOW GRANTS ON grant_ordering_table FOR grant_ordering_user
+----
+database_name  schema_name  table_name            grantee              privilege_type  is_grantable
+test           public       grant_ordering_table  grant_ordering_user  ALL             false
+test           public       grant_ordering_table  grant_ordering_user  SELECT          true
+
+statement ok
+REVOKE GRANT OPTION FOR ALL ON TABLE grant_ordering_table FROM grant_ordering_user
+
+query TTTTTB colnames
+SHOW GRANTS ON grant_ordering_table FOR grant_ordering_user
+----
+database_name  schema_name  table_name            grantee              privilege_type  is_grantable
+test           public       grant_ordering_table  grant_ordering_user  ALL             false
+
+# has the effect of granting WITH GRANT OPTION to ALL because of GRANT privilege via ALL
+statement ok
+GRANT UPDATE ON TABLE grant_ordering_table TO grant_ordering_user
+
+query TTTTTB colnames
+SHOW GRANTS ON grant_ordering_table FOR grant_ordering_user
+----
+database_name  schema_name  table_name            grantee              privilege_type  is_grantable
+test           public       grant_ordering_table  grant_ordering_user  ALL             true