sql: support ALTER DEFAULT PRIVILEGES IN SCHEMA

[RichardJCai]

Release note (sql change):
Support ALTER DEFAULT PRIVILEGES IN SCHEMA <schemas...>
Users can now in addition to specifying default privileges globally
(within a database), specify default privileges in a specific schema.

When creating an object that has default privileges specified at
the database (global) and at the schema level, the union of the
default privileges are taken.

Resolves #67376

[cockroach-teamcity]

This change is 

[rafiss]

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

---

docs/generated/eventlog.md, line 1562 at r1 (raw file):

| `RoleName` | Either role_name should be populated or for_all_roles should be true. The role having its default privileges altered. | yes |
| `ForAllRoles` | Identifies if FOR ALL ROLES is used. | no |
| `SchemaName` | The names of the affected schema. | yes |

nit: the field should be plural (SchemaNames) if it's a list

---

pkg/sql/drop_role.go, line 426 at r1 (raw file):

// accumulateDependentDefaultPrivileges checks for any default privileges
// that the users in userNames have and append them to the objectAndType array.
// Only one of db or sc should be non-nil.

this is a little brittle. why don't we instead make the parameter a catalog.DefaultPrivilegeDescriptor so that the caller has to call GetDefaultPrivilegeDescriptor?

---

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

}

// Calls expandPrivileges before calling the function passed in and calls

nit: the function comment should be a full sentence, and should start with grantOrRevokeDefaultPrivilegesHelper calls ...

---

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

// the default privilege descriptor from the database or both the db and schema.
func CreatePrivilegesFromDefaultPrivileges(
	defaultPrivilegeDescriptors []catalog.DefaultPrivilegeDescriptor,

why use a slice here instead of passing in one arg for the dbDefaultPrivs and another arg for schemaDefaultPrivs

---

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

	for _, tc := range testCases {
		defaultPrivilegeDescriptor := MakeNewDatabaseDefaultPrivilegeDescriptor()

after seeing this a few times, i wonder if it would be more readable to have one MakeDefaultPrivilegeDescriptor that accepts a descriptorType parameter?

also nit: MakeNewXyz is redundant; i think MakeXyz is sufficient.

---

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

		// for a schema.
		if newPrivileges.CheckPrivilege(creatorUser, privilege.ALL) {
			t.Errorf("expected creator to have not ALL privileges on %s", targetObject)

nit: "to have not ALL" is confusing. is there a typo?

also, just to clarify, this check is different than the "base" schema privileges that the "public" role has?

---

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

  option (gogoproto.equal) = true;
  repeated DefaultPrivilegesForRole default_privileges_per_role = 1 [(gogoproto.nullable) = false];
  enum DefaultPrivilegeDescriptorType {

hm, since we're adding this to the proto, does that mean all the logic that looks at this enum needs to be version gated? maybe it can be avoided if the deserialization logic defaults to setting this to 0?

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 120 at r1 (raw file):

statement ok
ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA s, public REVOKE ALL ON TABLES FROM testuser2;
ALTER DEFAULT PRIVILEGES FOR ALL ROLES IN SCHEMA s, public GRANT SELECT ON TABLES TO testuser2;

just to confirm, does this cover the case where default privs on (db+schema) overrides default privs on db? and make sure the override is actually per-schema?

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 161 at r1 (raw file):

test           public       t9          admin      ALL
test           public       t9          root       ALL
test           public       t9          testuser   CREATE

i think this is unrelated to your current work, but what does CREATE really mean here? isn't CREATE a DB privilege? so why are schema and table_name set?

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 193 at r1 (raw file):

# Default privileges for schemas have no defaults - no privileges are defined
# initially.

i thought we had CREATE granted to public by default? (ref #70266)

---

pkg/util/log/eventpb/privilege_events.proto, line 138 at r1 (raw file):

  // Identifies if FOR ALL ROLES is used.
  bool for_all_roles = 6 [(gogoproto.jsontag) = ",omitempty"];
  // The names of the affected schema.

nit: should be name not names

[RichardJCai]

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

---

docs/generated/eventlog.md, line 1562 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: the field should be plural (SchemaNames) if it's a list

Oops this actual should just be one schema right now.

---

pkg/sql/drop_role.go, line 426 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this is a little brittle. why don't we instead make the parameter a catalog.DefaultPrivilegeDescriptor so that the caller has to call GetDefaultPrivilegeDescriptor?

That's much nicer good call.

---

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

Previously, rafiss (Rafi Shamim) wrote…

nit: the function comment should be a full sentence, and should start with grantOrRevokeDefaultPrivilegesHelper calls ...

Done

---

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

Previously, rafiss (Rafi Shamim) wrote…

why use a slice here instead of passing in one arg for the dbDefaultPrivs and another arg for schemaDefaultPrivs

Good call, done.

---

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

Previously, rafiss (Rafi Shamim) wrote…

after seeing this a few times, i wonder if it would be more readable to have one MakeDefaultPrivilegeDescriptor that accepts a descriptorType parameter?

also nit: MakeNewXyz is redundant; i think MakeXyz is sufficient.

Done

---

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

Previously, rafiss (Rafi Shamim) wrote…

nit: "to have not ALL" is confusing. is there a typo?

also, just to clarify, this check is different than the "base" schema privileges that the "public" role has?

Reworded to creator should not have ALL privileges

This check is different.
I checked with PG there is no "default" / preset privileges for default privileges on schemas.

---

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

Previously, rafiss (Rafi Shamim) wrote…

hm, since we're adding this to the proto, does that mean all the logic that looks at this enum needs to be version gated? maybe it can be avoided if the deserialization logic defaults to setting this to 0?

I think the DefaultPrivilegeDescriptorType will resolve to 0 on older versions so that should be okay. I'll double check.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 120 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

just to confirm, does this cover the case where default privs on (db+schema) overrides default privs on db? and make sure the override is actually per-schema?

It's not override it's actually the union of the privileges on the db and schema

# When creating an object, take the union of the default privileges on
# the schema and the database.
# In the following test cases, testuser2 should have INSERT and SELECT.
statement ok
ALTER DEFAULT PRIVILEGES FOR ROLE testuser GRANT INSERT ON TABLES TO testuser2

statement ok
CREATE TABLE t2()

query TTTTT colnames
SHOW GRANTS ON t2
----
database_name  schema_name  table_name  grantee    privilege_type
test           public       t2          admin      ALL
test           public       t2          root       ALL
test           public       t2          testuser   ALL
test           public       t2          testuser2  INSERT
test           public       t2          testuser2  SELECT

this case covers that

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 161 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this is unrelated to your current work, but what does CREATE really mean here? isn't CREATE a DB privilege? so why are schema and table_name set?

I think CREATE on table was used to substitute for some owner privileges, ie since we didn't have the concept of ownership before, you needed CREATE to alter or drop the table. CREATE still lets you do those things.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 193 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i thought we had CREATE granted to public by default? (ref #70266)

I believe thats on the public schema

---

pkg/util/log/eventpb/privilege_events.proto, line 138 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should be name not names

Done

[rafiss]

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

---

docs/generated/eventlog.md, line 1562 at r2 (raw file):

| `RoleName` | Either role_name should be populated or for_all_roles should be true. The role having its default privileges altered. | yes |
| `ForAllRoles` | Identifies if FOR ALL ROLES is used. | no |
| `SchemaName` | The names of the affected schema. | yes |

nit: should be "The name of the affected schema"

---

pkg/sql/drop_role.go, line 426 at r2 (raw file):

// accumulateDependentDefaultPrivileges checks for any default privileges
// that the users in userNames have and append them to the objectAndType array.
// Only one of db or sc should be non-nil.

nit: update comment that describes the params

---

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

Previously, RichardJCai (Richard Cai) wrote…

I think the DefaultPrivilegeDescriptorType will resolve to 0 on older versions so that should be okay. I'll double check.

is there already a mixed-version test for this? i think that should be sufficient

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 193 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I believe thats on the public schema

ah right, so that would be seen from show grants on schema public; right? i think we'd want a test for that too, although i guess that might be considered out of scope for this PR. would you be open to adding that as a separate commit?

[RichardJCai]

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

---

docs/generated/eventlog.md, line 1562 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should be "The name of the affected schema"

It was updated in r3 I think you saw an older commit.

---

pkg/sql/drop_role.go, line 426 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: update comment that describes the params

Oops done

---

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

Previously, rafiss (Rafi Shamim) wrote…

is there already a mixed-version test for this? i think that should be sufficient

func TestUnsetDefaultPrivilegeDescriptorType(t *testing.T) {
	emptyDefaultPrivilegeDescriptor := descpb.DefaultPrivilegeDescriptor{}

	// If Type is not set, it should resolve to
	// descpb.DefaultPrivilegeDescriptor_DATABASE by default.
	require.Equal(t, emptyDefaultPrivilegeDescriptor.Type, descpb.DefaultPrivilegeDescriptor_DATABASE)
}

is this test sufficient?

I think a mixed-version test is overkill and a little unclear.

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 193 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah right, so that would be seen from show grants on schema public; right? i think we'd want a test for that too, although i guess that might be considered out of scope for this PR. would you be open to adding that as a separate commit?

Yeah I don't think that is relevant to this PR as it's not handled as a default privilege at all.
Right now that's hardcoded in descriptor.go

I can add a test for that in another logic test if you want.

	publicSchemaID, err := catalogkv.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec)
	if err != nil {
		return descpb.InvalidID, err
	}

	// Every database must be initialized with the public schema.
	// Create the SchemaDescriptor.
	// In postgres, the user "postgres" is the owner of the public schema in a
	// newly created db. Postgres and Public have USAGE and CREATE privileges.
	// In CockroachDB, root is our substitute for the postgres user.
	publicSchemaPrivileges := descpb.NewBasePrivilegeDescriptor(security.AdminRoleName())
	// By default, everyone has USAGE and CREATE on the public schema.
	publicSchemaPrivileges.Grant(security.PublicRoleName(), privilege.List{privilege.CREATE, privilege.USAGE}, false)
	publicSchemaDesc := schemadesc.NewBuilder(&descpb.SchemaDescriptor{
		ParentID:   dbID,
		Name:       tree.PublicSchema,
		ID:         publicSchemaID,
		Privileges: publicSchemaPrivileges,
		Version:    1,
	}).BuildCreatedMutableSchema()

[rafiss]

lgtm! but can you add in the error propagation?

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

---

pkg/sql/drop_role.go, line 432 at r4 (raw file):

	// The func we pass into ForEachDefaultPrivilegeForRole will never
	// err so no err will be returned.
	_ = defaultPrivilegeDescriptor.ForEachDefaultPrivilegeForRole(func(

didn't realize the error was getting swallowed here. even though we don't need it know it is more future-proof to propagate the error up (so accumulateDependentDefaultPrivileges should return an error). otherwise a future refactoring could easily lead to bad error handling.

---

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

Previously, RichardJCai (Richard Cai) wrote…

func TestUnsetDefaultPrivilegeDescriptorType(t *testing.T) {
	emptyDefaultPrivilegeDescriptor := descpb.DefaultPrivilegeDescriptor{}

	// If Type is not set, it should resolve to
	// descpb.DefaultPrivilegeDescriptor_DATABASE by default.
	require.Equal(t, emptyDefaultPrivilegeDescriptor.Type, descpb.DefaultPrivilegeDescriptor_DATABASE)
}

is this test sufficient?

I think a mixed-version test is overkill and a little unclear.

this is sufficient, thanks!

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 120 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

It's not override it's actually the union of the privileges on the db and schema

# When creating an object, take the union of the default privileges on
# the schema and the database.
# In the following test cases, testuser2 should have INSERT and SELECT.
statement ok
ALTER DEFAULT PRIVILEGES FOR ROLE testuser GRANT INSERT ON TABLES TO testuser2

statement ok
CREATE TABLE t2()

query TTTTT colnames
SHOW GRANTS ON t2
----
database_name  schema_name  table_name  grantee    privilege_type
test           public       t2          admin      ALL
test           public       t2          root       ALL
test           public       t2          testuser   ALL
test           public       t2          testuser2  INSERT
test           public       t2          testuser2  SELECT

this case covers that

got it thanks for explaining

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 161 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think CREATE on table was used to substitute for some owner privileges, ie since we didn't have the concept of ownership before, you needed CREATE to alter or drop the table. CREATE still lets you do those things.

done

---

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema, line 193 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Yeah I don't think that is relevant to this PR as it's not handled as a default privilege at all.
Right now that's hardcoded in descriptor.go

I can add a test for that in another logic test if you want.

	publicSchemaID, err := catalogkv.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec)
	if err != nil {
		return descpb.InvalidID, err
	}

	// Every database must be initialized with the public schema.
	// Create the SchemaDescriptor.
	// In postgres, the user "postgres" is the owner of the public schema in a
	// newly created db. Postgres and Public have USAGE and CREATE privileges.
	// In CockroachDB, root is our substitute for the postgres user.
	publicSchemaPrivileges := descpb.NewBasePrivilegeDescriptor(security.AdminRoleName())
	// By default, everyone has USAGE and CREATE on the public schema.
	publicSchemaPrivileges.Grant(security.PublicRoleName(), privilege.List{privilege.CREATE, privilege.USAGE}, false)
	publicSchemaDesc := schemadesc.NewBuilder(&descpb.SchemaDescriptor{
		ParentID:   dbID,
		Name:       tree.PublicSchema,
		ID:         publicSchemaID,
		Privileges: publicSchemaPrivileges,
		Version:    1,
	}).BuildCreatedMutableSchema()

actually i'll just make a PR to add it so you don't have to context switch

[RichardJCai]

TFTR
bors r=rafiss

[craig]

Build succeeded:

• GitHub CI (Cockroach)