Merge #107317

107317: sql: functions have EXECUTE priv for public by default r=rafiss a=rafiss This uses the default privilege infrastructure in a similar way to how the USAGE privilege for types is given to `public` by default. This also fixes a display bug that was preventing the ALL privileges that are given to root and admin from being shown in SHOW. informs #103842 A complete fix for that issue may also require an upgrade migration to give existing functions the EXECUTE privilege for public. Release note (sql change): The public pseudo-role now receives the EXECUTE privilege by default for all user-defined functions that are created. This can be adjusted by using ALTER DEFAULT PRIVILEGES. Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · Jul 21, 2023 · e0235d0 · e0235d0
2 parents 3eaddac + c9d7737
commit e0235d0
Show file tree

Hide file tree

Showing 36 changed files with 998 additions and 354 deletions.
diff --git a/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions b/pkg/ccl/backupccl/testdata/backup-restore/backup-permissions
@@ -295,8 +295,7 @@ BACKUP INTO 'userfile:///test-nonroot-cluster';
 exec-sql user=testuser
 BACKUP DATABASE d_test_fn INTO 'userfile:///test-nonroot-db';
 ----
-pq: user testuser does not have EXECUTE privilege on function f
-HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here https://www.cockroachlabs.com/docs/stable/backup.html#required-privileges. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d_test_fn.
+NOTICE: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here https://www.cockroachlabs.com/docs/stable/backup.html#required-privileges. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d_test_fn.
 
 exec-sql
 REVOKE SYSTEM BACKUP FROM testuser;

diff --git a/pkg/sql/catalog/catpb/default_privilege.go b/pkg/sql/catalog/catpb/default_privilege.go
@@ -107,7 +107,8 @@ func InitDefaultPrivilegesForRole(
 	if role.ForAllRoles {
 		defaultPrivilegesRole = &DefaultPrivilegesForRole_ForAllRoles{
 			ForAllRoles: &DefaultPrivilegesForRole_ForAllRolesPseudoRole{
-				PublicHasUsageOnTypes: true,
+				PublicHasUsageOnTypes:       true,
+				PublicHasExecuteOnFunctions: true,
 			},
 		}
 		return DefaultPrivilegesForRole{
@@ -121,6 +122,7 @@ func InitDefaultPrivilegesForRole(
 			ExplicitRole: &DefaultPrivilegesForRole_ExplicitRole{
 				UserProto:                       role.Role.EncodeProto(),
 				PublicHasUsageOnTypes:           true,
+				PublicHasExecuteOnFunctions:     true,
 				RoleHasAllPrivilegesOnTables:    true,
 				RoleHasAllPrivilegesOnSequences: true,
 				RoleHasAllPrivilegesOnSchemas:   true,

diff --git a/pkg/sql/catalog/catpb/privilege.proto b/pkg/sql/catalog/catpb/privilege.proto
@@ -67,6 +67,7 @@ message DefaultPrivilegesForRole {
     optional bool role_has_all_privileges_on_schemas = 7 [(gogoproto.nullable) = false];
     optional bool role_has_all_privileges_on_types = 8 [(gogoproto.nullable) = false];
     optional bool role_has_all_privileges_on_functions = 9 [(gogoproto.nullable) = false];
+    optional bool public_has_execute_on_functions = 10 [(gogoproto.nullable) = false];
   }
   // ForAllRoles represents when default privileges are defined
   // using FOR ALL ROLES.
@@ -76,6 +77,7 @@ message DefaultPrivilegesForRole {
     // role has privileges on tables/sequences/schemas and types as
     // for_all_roles is not a real role and cannot have grants.
     optional bool public_has_usage_on_types = 11 [(gogoproto.nullable) = false];
+    optional bool public_has_execute_on_functions = 12 [(gogoproto.nullable) = false];
   }
   oneof role {
     ExplicitRole explicit_role = 12;

diff --git a/pkg/sql/catalog/catprivilege/default_privilege.go b/pkg/sql/catalog/catprivilege/default_privilege.go
@@ -157,8 +157,9 @@ func (d *Mutable) RevokeDefaultPrivileges(
 		(!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Tables) ||
 			!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Sequences) ||
 			!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Types) ||
-			!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Schemas)) ||
-		!GetPublicHasUsageOnTypes(defaultPrivilegesForRole) {
+			!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Schemas) ||
+			!GetPublicHasUsageOnTypes(defaultPrivilegesForRole) ||
+			!GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole)) {
 		return nil
 	}
 
@@ -334,6 +335,19 @@ func foldPrivileges(
 			return err
 		}
 	}
+	if targetObject == privilege.Functions &&
+		privileges.CheckPrivilege(username.PublicRoleName(), privilege.EXECUTE) {
+		setPublicHasExecuteOnFunctions(defaultPrivilegesForRole, true)
+		if err := privileges.Revoke(
+			username.PublicRoleName(),
+			privilege.List{privilege.EXECUTE},
+			privilege.Function,
+			false, /* grantOptionFor */
+		); err != nil {
+			return err
+		}
+	}
+
 	// ForAllRoles cannot be a grantee, nothing left to do.
 	if role.ForAllRoles {
 		return nil
@@ -368,6 +382,10 @@ func expandPrivileges(
 		privileges.Grant(username.PublicRoleName(), privilege.List{privilege.USAGE}, false /* withGrantOption */)
 		setPublicHasUsageOnTypes(defaultPrivilegesForRole, false)
 	}
+	if targetObject == privilege.Functions && GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole) {
+		privileges.Grant(username.PublicRoleName(), privilege.List{privilege.EXECUTE}, false /* withGrantOption */)
+		setPublicHasExecuteOnFunctions(defaultPrivilegesForRole, false)
+	}
 	// ForAllRoles cannot be a grantee, nothing left to do.
 	if role.ForAllRoles {
 		return
@@ -396,17 +414,31 @@ func GetUserPrivilegesForObject(
 			Privileges: privilege.USAGE.Mask(),
 		})
 	}
+	if GetPublicHasExecuteOnFunctions(&p) && targetObject == privilege.Functions {
+		userPrivileges = append(userPrivileges, catpb.UserPrivileges{
+			UserProto:  username.PublicRoleName().EncodeProto(),
+			Privileges: privilege.EXECUTE.Mask(),
+		})
+	}
 	return userPrivileges
 }
 
-// GetPublicHasUsageOnTypes returns whether Public has Usage privilege on types.
+// GetPublicHasUsageOnTypes returns whether Public has USAGE privilege on types.
 func GetPublicHasUsageOnTypes(defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole) bool {
 	if defaultPrivilegesForRole.IsExplicitRole() {
 		return defaultPrivilegesForRole.GetExplicitRole().PublicHasUsageOnTypes
 	}
 	return defaultPrivilegesForRole.GetForAllRoles().PublicHasUsageOnTypes
 }
 
+// GetPublicHasExecuteOnFunctions returns whether Public has EXECUTE privilege on functions.
+func GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole) bool {
+	if defaultPrivilegesForRole.IsExplicitRole() {
+		return defaultPrivilegesForRole.GetExplicitRole().PublicHasExecuteOnFunctions
+	}
+	return defaultPrivilegesForRole.GetForAllRoles().PublicHasExecuteOnFunctions
+}
+
 // GetRoleHasAllPrivilegesOnTargetObject returns whether the creator role
 // has all privileges on the default privileges target object.
 func GetRoleHasAllPrivilegesOnTargetObject(
@@ -443,6 +475,17 @@ func setPublicHasUsageOnTypes(
 	}
 }
 
+// setPublicHasExecuteOnFunctions sets PublicHasExecuteOnFunctions to publicHasExecuteOnFunctions.
+func setPublicHasExecuteOnFunctions(
+	defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole, publicHasExecuteOnFunctions bool,
+) {
+	if defaultPrivilegesForRole.IsExplicitRole() {
+		defaultPrivilegesForRole.GetExplicitRole().PublicHasExecuteOnFunctions = publicHasExecuteOnFunctions
+	} else {
+		defaultPrivilegesForRole.GetForAllRoles().PublicHasExecuteOnFunctions = publicHasExecuteOnFunctions
+	}
+}
+
 // applyDefaultPrivileges adds new privileges to this descriptor and new grant options which
 // could be different from the privileges. Unlike the normal grant, the privileges
 // and the grant options being granted could be different

diff --git a/pkg/sql/catalog/catprivilege/default_privilege_test.go b/pkg/sql/catalog/catprivilege/default_privilege_test.go
@@ -844,6 +844,45 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
 	if GetPublicHasUsageOnTypes(defaultPrivilegesForCreator) {
 		t.Errorf("expected public to not have USAGE privilege on types")
 	}
+
+	if err := defaultPrivileges.RevokeDefaultPrivileges(
+		catpb.DefaultPrivilegesRole{Role: creatorUser},
+		privilege.List{privilege.EXECUTE},
+		[]username.SQLUsername{username.PublicRoleName()},
+		privilege.Functions,
+		false, /* grantOptionFor */
+	); err != nil {
+		t.Fatal(err)
+	}
+	if GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
+		t.Errorf("expected public to not have EXECUTE privilege on functions")
+	}
+	if err := defaultPrivileges.GrantDefaultPrivileges(
+		catpb.DefaultPrivilegesRole{Role: creatorUser},
+		privilege.List{privilege.EXECUTE},
+		[]username.SQLUsername{username.PublicRoleName()},
+		privilege.Functions,
+		false, /* withGrantOption */
+	); err != nil {
+		t.Fatal(err)
+	}
+	if !GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
+		t.Errorf("expected public to have EXECUTE privilege on functions")
+	}
+
+	// Test a complete revoke afterwards.
+	if err := defaultPrivileges.RevokeDefaultPrivileges(
+		catpb.DefaultPrivilegesRole{Role: creatorUser},
+		privilege.List{privilege.EXECUTE},
+		[]username.SQLUsername{username.PublicRoleName()},
+		privilege.Functions,
+		false, /* grantOptionFor */
+	); err != nil {
+		t.Fatal(err)
+	}
+	if GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
+		t.Errorf("expected public to not have EXECUTE privilege on functions")
+	}
 }
 
 // TestApplyDefaultPrivileges tests whether granting potentially different privileges and grant options

diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -6361,6 +6361,20 @@ CREATE TABLE crdb_internal.default_privileges (
 									return err
 								}
 							}
+							if catprivilege.GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole) {
+								if err := addRow(
+									database,    // database_name
+									schema,      // schema_name
+									role,        // role
+									forAllRoles, // for_all_roles
+									tree.NewDString(privilege.Functions.String()),           // object_type
+									tree.NewDString(username.PublicRoleName().Normalized()), // grantee
+									tree.NewDString(privilege.EXECUTE.String()),             // privilege_type
+									tree.DBoolFalse, // is_grantable
+								); err != nil {
+									return err
+								}
+							}
 						}
 					}
 					return nil

diff --git a/pkg/sql/information_schema.go b/pkg/sql/information_schema.go
@@ -1748,6 +1748,7 @@ var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
 			dbNameStr := tree.NewDString(db.GetName())
 			exPriv := tree.NewDString(privilege.EXECUTE.String())
 			roleNameForBuiltins := []*tree.DString{
+				tree.NewDString(username.AdminRole),
 				tree.NewDString(username.RootUser),
 				tree.NewDString(username.PublicRole),
 			}
@@ -1773,7 +1774,7 @@ var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
 
 				_, overloads := builtinsregistry.GetBuiltinProperties(name)
 				for _, o := range overloads {
-					fnSpecificName := tree.NewDString(fmt.Sprintf("%s_%d", fnNameStr, o.Oid))
+					fnSpecificName := tree.NewDString(nameConcatOid(fnNameStr, o.Oid))
 					for _, grantee := range roleNameForBuiltins {
 						if err := addRow(
 							tree.DNull, // grantor
@@ -1810,42 +1811,39 @@ var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
 					if !canSeeDescriptor {
 						return nil
 					}
-					privs := fn.GetPrivileges()
-					scNameStr := tree.NewDString(sc.GetName())
-					fnSpecificName := tree.NewDString(fmt.Sprintf("%s_%d", fn.GetName(), catid.FuncIDToOID(fn.GetID())))
-					fnName := tree.NewDString(fn.GetName())
-					// EXECUTE is the only privilege kind relevant to functions.
-					if err := addRow(
-						tree.DNull, // grantor
-						tree.NewDString(privs.Owner().Normalized()), // grantee
-						dbNameStr,      // specific_catalog
-						scNameStr,      // specific_schema
-						fnSpecificName, // specific_name
-						dbNameStr,      // routine_catalog
-						scNameStr,      // routine_schema
-						fnName,         // routine_name
-						exPriv,         // privilege_type
-						yesString,      // is_grantable
-					); err != nil {
+					privs, err := fn.GetPrivileges().Show(privilege.Function, true /* showImplicitOwnerPrivs */)
+					if err != nil {
 						return err
 					}
-					for _, user := range privs.Users {
-						if !privilege.EXECUTE.IsSetIn(user.Privileges) {
-							continue
-						}
-						if err := addRow(
-							tree.DNull, // grantor
-							tree.NewDString(user.User().Normalized()), // grantee
-							dbNameStr,      // specific_catalog
-							scNameStr,      // specific_schema
-							fnSpecificName, // specific_name
-							dbNameStr,      // routine_catalog
-							scNameStr,      // routine_schema
-							fnName,         // routine_name
-							exPriv,         // privilege_type
-							yesOrNoDatum(privilege.EXECUTE.IsSetIn(user.WithGrantOption)), // is_grantable
-						); err != nil {
-							return err
+					scNameStr := tree.NewDString(sc.GetName())
+
+					fnSpecificName := tree.NewDString(nameConcatOid(fn.GetName(), catid.FuncIDToOID(fn.GetID())))
+					fnName := tree.NewDString(fn.GetName())
+					for _, u := range privs {
+						userNameStr := tree.NewDString(u.User.Normalized())
+						for _, priv := range u.Privileges {
+							// We use this function to check for the grant option so that the
+							// object owner also gets is_grantable=true.
+							isGrantable, err := p.CheckGrantOptionsForUser(
+								ctx, fn.GetPrivileges(), sc, []privilege.Kind{priv.Kind}, u.User,
+							)
+							if err != nil {
+								return err
+							}
+							if err := addRow(
+								tree.DNull,                          // grantor
+								userNameStr,                         // grantee
+								dbNameStr,                           // specific_catalog
+								scNameStr,                           // specific_schema
+								fnSpecificName,                      // specific_name
+								dbNameStr,                           // routine_catalog
+								scNameStr,                           // routine_schema
+								fnName,                              // routine_name
+								tree.NewDString(priv.Kind.String()), // privilege_type
+								yesOrNoDatum(isGrantable),           // is_grantable
+							); err != nil {
+								return err
+							}
 						}
 					}
 					return nil
@@ -2958,3 +2956,16 @@ func userCanSeeDescriptor(
 func descriptorIsVisible(desc catalog.Descriptor, allowAdding bool) bool {
 	return desc.Public() || (allowAdding && desc.Adding())
 }
+
+// nameConcatOid is a Go version of the nameconcatoid builtin function. The
+// result is the same as fmt.Sprintf("%s_%d", s, o) except that, if it would not
+// fit in 63 characters, we make it do so by truncating the name input (not the
+// oid).
+func nameConcatOid(s string, o oid.Oid) string {
+	const maxLen = 63
+	oidStr := strconv.Itoa(int(o))
+	if len(s)+1+len(oidStr) <= maxLen {
+		return s + "_" + oidStr
+	}
+	return s[:maxLen-1-len(oidStr)] + "_" + oidStr
+}