Skip to content

Commit 969b575

Browse files
rafissrafiss
committed
sql: functions have EXECUTE priv for public by default
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. 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.
1 parent b6297be commit 969b575

34 files changed

+976
-351
lines changed

‎pkg/ccl/backupccl/testdata/backup-restore/backup-permissions‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,7 @@ BACKUP INTO 'userfile:///test-nonroot-cluster';
295295
exec-sql user=testuser
296296
BACKUP DATABASE d_test_fn INTO 'userfile:///test-nonroot-db';
297297
----
298-
pq: user testuser does not have EXECUTE privilege on function f
299-
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.
298+
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.
300299

301300
exec-sql
302301
REVOKE SYSTEM BACKUP FROM testuser;

‎pkg/sql/catalog/catpb/default_privilege.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ func InitDefaultPrivilegesForRole(
107107
if role.ForAllRoles {
108108
defaultPrivilegesRole = &DefaultPrivilegesForRole_ForAllRoles{
109109
ForAllRoles: &DefaultPrivilegesForRole_ForAllRolesPseudoRole{
110-
PublicHasUsageOnTypes: true,
110+
PublicHasUsageOnTypes: true,
111+
PublicHasExecuteOnFunctions: true,
111112
},
112113
}
113114
return DefaultPrivilegesForRole{
@@ -121,6 +122,7 @@ func InitDefaultPrivilegesForRole(
121122
ExplicitRole: &DefaultPrivilegesForRole_ExplicitRole{
122123
UserProto: role.Role.EncodeProto(),
123124
PublicHasUsageOnTypes: true,
125+
PublicHasExecuteOnFunctions: true,
124126
RoleHasAllPrivilegesOnTables: true,
125127
RoleHasAllPrivilegesOnSequences: true,
126128
RoleHasAllPrivilegesOnSchemas: true,

‎pkg/sql/catalog/catpb/privilege.proto‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ message DefaultPrivilegesForRole {
6767
optional bool role_has_all_privileges_on_schemas = 7 [(gogoproto.nullable) = false];
6868
optional bool role_has_all_privileges_on_types = 8 [(gogoproto.nullable) = false];
6969
optional bool role_has_all_privileges_on_functions = 9 [(gogoproto.nullable) = false];
70+
optional bool public_has_execute_on_functions = 10 [(gogoproto.nullable) = false];
7071
}
7172
// ForAllRoles represents when default privileges are defined
7273
// using FOR ALL ROLES.
@@ -76,6 +77,7 @@ message DefaultPrivilegesForRole {
7677
// role has privileges on tables/sequences/schemas and types as
7778
// for_all_roles is not a real role and cannot have grants.
7879
optional bool public_has_usage_on_types = 11 [(gogoproto.nullable) = false];
80+
optional bool public_has_execute_on_functions = 12 [(gogoproto.nullable) = false];
7981
}
8082
oneof role {
8183
ExplicitRole explicit_role = 12;

‎pkg/sql/catalog/catprivilege/default_privilege.go‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,9 @@ func (d *Mutable) RevokeDefaultPrivileges(
157157
(!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Tables) ||
158158
!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Sequences) ||
159159
!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Types) ||
160-
!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Schemas)) ||
161-
!GetPublicHasUsageOnTypes(defaultPrivilegesForRole) {
160+
!GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForRole, privilege.Schemas) ||
161+
!GetPublicHasUsageOnTypes(defaultPrivilegesForRole) ||
162+
!GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole)) {
162163
return nil
163164
}
164165

@@ -334,6 +335,19 @@ func foldPrivileges(
334335
return err
335336
}
336337
}
338+
if targetObject == privilege.Functions &&
339+
privileges.CheckPrivilege(username.PublicRoleName(), privilege.EXECUTE) {
340+
setPublicHasExecuteOnFunctions(defaultPrivilegesForRole, true)
341+
if err := privileges.Revoke(
342+
username.PublicRoleName(),
343+
privilege.List{privilege.EXECUTE},
344+
privilege.Function,
345+
false, /* grantOptionFor */
346+
); err != nil {
347+
return err
348+
}
349+
}
350+
337351
// ForAllRoles cannot be a grantee, nothing left to do.
338352
if role.ForAllRoles {
339353
return nil
@@ -368,6 +382,10 @@ func expandPrivileges(
368382
privileges.Grant(username.PublicRoleName(), privilege.List{privilege.USAGE}, false /* withGrantOption */)
369383
setPublicHasUsageOnTypes(defaultPrivilegesForRole, false)
370384
}
385+
if targetObject == privilege.Functions && GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole) {
386+
privileges.Grant(username.PublicRoleName(), privilege.List{privilege.EXECUTE}, false /* withGrantOption */)
387+
setPublicHasExecuteOnFunctions(defaultPrivilegesForRole, false)
388+
}
371389
// ForAllRoles cannot be a grantee, nothing left to do.
372390
if role.ForAllRoles {
373391
return
@@ -396,17 +414,31 @@ func GetUserPrivilegesForObject(
396414
Privileges: privilege.USAGE.Mask(),
397415
})
398416
}
417+
if GetPublicHasExecuteOnFunctions(&p) && targetObject == privilege.Functions {
418+
userPrivileges = append(userPrivileges, catpb.UserPrivileges{
419+
UserProto: username.PublicRoleName().EncodeProto(),
420+
Privileges: privilege.EXECUTE.Mask(),
421+
})
422+
}
399423
return userPrivileges
400424
}
401425

402-
// GetPublicHasUsageOnTypes returns whether Public has Usage privilege on types.
426+
// GetPublicHasUsageOnTypes returns whether Public has USAGE privilege on types.
403427
func GetPublicHasUsageOnTypes(defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole) bool {
404428
if defaultPrivilegesForRole.IsExplicitRole() {
405429
return defaultPrivilegesForRole.GetExplicitRole().PublicHasUsageOnTypes
406430
}
407431
return defaultPrivilegesForRole.GetForAllRoles().PublicHasUsageOnTypes
408432
}
409433

434+
// GetPublicHasExecuteOnFunctions returns whether Public has EXECUTE privilege on functions.
435+
func GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole) bool {
436+
if defaultPrivilegesForRole.IsExplicitRole() {
437+
return defaultPrivilegesForRole.GetExplicitRole().PublicHasExecuteOnFunctions
438+
}
439+
return defaultPrivilegesForRole.GetForAllRoles().PublicHasExecuteOnFunctions
440+
}
441+
410442
// GetRoleHasAllPrivilegesOnTargetObject returns whether the creator role
411443
// has all privileges on the default privileges target object.
412444
func GetRoleHasAllPrivilegesOnTargetObject(
@@ -443,6 +475,17 @@ func setPublicHasUsageOnTypes(
443475
}
444476
}
445477

478+
// setPublicHasExecuteOnFunctions sets PublicHasExecuteOnFunctions to publicHasExecuteOnFunctions.
479+
func setPublicHasExecuteOnFunctions(
480+
defaultPrivilegesForRole *catpb.DefaultPrivilegesForRole, publicHasExecuteOnFunctions bool,
481+
) {
482+
if defaultPrivilegesForRole.IsExplicitRole() {
483+
defaultPrivilegesForRole.GetExplicitRole().PublicHasExecuteOnFunctions = publicHasExecuteOnFunctions
484+
} else {
485+
defaultPrivilegesForRole.GetForAllRoles().PublicHasExecuteOnFunctions = publicHasExecuteOnFunctions
486+
}
487+
}
488+
446489
// applyDefaultPrivileges adds new privileges to this descriptor and new grant options which
447490
// could be different from the privileges. Unlike the normal grant, the privileges
448491
// and the grant options being granted could be different

‎pkg/sql/catalog/catprivilege/default_privilege_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -844,6 +844,45 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
844844
if GetPublicHasUsageOnTypes(defaultPrivilegesForCreator) {
845845
t.Errorf("expected public to not have USAGE privilege on types")
846846
}
847+
848+
if err := defaultPrivileges.RevokeDefaultPrivileges(
849+
catpb.DefaultPrivilegesRole{Role: creatorUser},
850+
privilege.List{privilege.EXECUTE},
851+
[]username.SQLUsername{username.PublicRoleName()},
852+
privilege.Functions,
853+
false, /* grantOptionFor */
854+
); err != nil {
855+
t.Fatal(err)
856+
}
857+
if GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
858+
t.Errorf("expected public to not have EXECUTE privilege on functions")
859+
}
860+
if err := defaultPrivileges.GrantDefaultPrivileges(
861+
catpb.DefaultPrivilegesRole{Role: creatorUser},
862+
privilege.List{privilege.EXECUTE},
863+
[]username.SQLUsername{username.PublicRoleName()},
864+
privilege.Functions,
865+
false, /* withGrantOption */
866+
); err != nil {
867+
t.Fatal(err)
868+
}
869+
if !GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
870+
t.Errorf("expected public to have EXECUTE privilege on functions")
871+
}
872+
873+
// Test a complete revoke afterwards.
874+
if err := defaultPrivileges.RevokeDefaultPrivileges(
875+
catpb.DefaultPrivilegesRole{Role: creatorUser},
876+
privilege.List{privilege.EXECUTE},
877+
[]username.SQLUsername{username.PublicRoleName()},
878+
privilege.Functions,
879+
false, /* grantOptionFor */
880+
); err != nil {
881+
t.Fatal(err)
882+
}
883+
if GetPublicHasExecuteOnFunctions(defaultPrivilegesForCreator) {
884+
t.Errorf("expected public to not have EXECUTE privilege on functions")
885+
}
847886
}
848887

849888
// TestApplyDefaultPrivileges tests whether granting potentially different privileges and grant options

‎pkg/sql/crdb_internal.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6361,6 +6361,20 @@ CREATE TABLE crdb_internal.default_privileges (
63616361
return err
63626362
}
63636363
}
6364+
if catprivilege.GetPublicHasExecuteOnFunctions(defaultPrivilegesForRole) {
6365+
if err := addRow(
6366+
database, // database_name
6367+
schema, // schema_name
6368+
role, // role
6369+
forAllRoles, // for_all_roles
6370+
tree.NewDString(privilege.Functions.String()), // object_type
6371+
tree.NewDString(username.PublicRoleName().Normalized()), // grantee
6372+
tree.NewDString(privilege.EXECUTE.String()), // privilege_type
6373+
tree.DBoolFalse, // is_grantable
6374+
); err != nil {
6375+
return err
6376+
}
6377+
}
63646378
}
63656379
}
63666380
return nil

‎pkg/sql/information_schema.go‎

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,6 +1748,7 @@ var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
17481748
dbNameStr := tree.NewDString(db.GetName())
17491749
exPriv := tree.NewDString(privilege.EXECUTE.String())
17501750
roleNameForBuiltins := []*tree.DString{
1751+
tree.NewDString(username.AdminRole),
17511752
tree.NewDString(username.RootUser),
17521753
tree.NewDString(username.PublicRole),
17531754
}
@@ -1810,42 +1811,38 @@ var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
18101811
if !canSeeDescriptor {
18111812
return nil
18121813
}
1813-
privs := fn.GetPrivileges()
1814+
privs, err := fn.GetPrivileges().Show(privilege.Function, true /* showImplicitOwnerPrivs */)
1815+
if err != nil {
1816+
return err
1817+
}
18141818
scNameStr := tree.NewDString(sc.GetName())
18151819
fnSpecificName := tree.NewDString(fmt.Sprintf("%s_%d", fn.GetName(), catid.FuncIDToOID(fn.GetID())))
18161820
fnName := tree.NewDString(fn.GetName())
1817-
// EXECUTE is the only privilege kind relevant to functions.
1818-
if err := addRow(
1819-
tree.DNull, // grantor
1820-
tree.NewDString(privs.Owner().Normalized()), // grantee
1821-
dbNameStr, // specific_catalog
1822-
scNameStr, // specific_schema
1823-
fnSpecificName, // specific_name
1824-
dbNameStr, // routine_catalog
1825-
scNameStr, // routine_schema
1826-
fnName, // routine_name
1827-
exPriv, // privilege_type
1828-
yesString, // is_grantable
1829-
); err != nil {
1830-
return err
1831-
}
1832-
for _, user := range privs.Users {
1833-
if !privilege.EXECUTE.IsSetIn(user.Privileges) {
1834-
continue
1835-
}
1836-
if err := addRow(
1837-
tree.DNull, // grantor
1838-
tree.NewDString(user.User().Normalized()), // grantee
1839-
dbNameStr, // specific_catalog
1840-
scNameStr, // specific_schema
1841-
fnSpecificName, // specific_name
1842-
dbNameStr, // routine_catalog
1843-
scNameStr, // routine_schema
1844-
fnName, // routine_name
1845-
exPriv, // privilege_type
1846-
yesOrNoDatum(privilege.EXECUTE.IsSetIn(user.WithGrantOption)), // is_grantable
1847-
); err != nil {
1848-
return err
1821+
for _, u := range privs {
1822+
userNameStr := tree.NewDString(u.User.Normalized())
1823+
for _, priv := range u.Privileges {
1824+
// We use this function to check for the grant option so that the
1825+
// object owner also gets is_grantable=true.
1826+
isGrantable, err := p.CheckGrantOptionsForUser(
1827+
ctx, fn.GetPrivileges(), sc, []privilege.Kind{priv.Kind}, u.User,
1828+
)
1829+
if err != nil {
1830+
return err
1831+
}
1832+
if err := addRow(
1833+
tree.DNull, // grantor
1834+
userNameStr, // grantee
1835+
dbNameStr, // specific_catalog
1836+
scNameStr, // specific_schema
1837+
fnSpecificName, // specific_name
1838+
dbNameStr, // routine_catalog
1839+
scNameStr, // routine_schema
1840+
fnName, // routine_name
1841+
tree.NewDString(priv.Kind.String()), // privilege_type
1842+
yesOrNoDatum(isGrantable), // is_grantable
1843+
); err != nil {
1844+
return err
1845+
}
18491846
}
18501847
}
18511848
return nil

0 commit comments

Comments
 (0)