sql: support default privileges at the database level

[RichardJCai]

fixes #65604

sql: support default privileges at the database level

This only adds default privileges stored on the database, we can add support for storing default privileges on schemas afterwards (shouldn't be a huge add onto this). Hopefully we can wait for #55793 so we don't need special logic to handle the public schema.

This PR does not yet address handling having USAGE on types as a default privilege for the public role.

Migration is not addressed yet, this PR still maintains backwards compatibility with how we "inherited" privileges before.

Also sorry to the reviewers about the size of the PR, a lot of lines do come from tests however, specifically parse test. Hopefully it shouldn't be too bad.

Release note (sql change): Added support for ALTER DEFAULT PRIVILEGES
and default privileges stored on databases.

All objects created in a database will have the privilege set defined
by the default privileges for that type of object on the database.
The types of objects are TABLES, SEQUENCES, SCHEMAS, TYPES.

Example: ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO foo
makes it such that all tables created by the user that executed the
ALTER DEFAULT PRIVILEGES command will have SELECT privilege on the table
for user foo.

Additionally, one can specify a role.
Example: ALTER DEFAULT PRIVILEGES FOR ROLE bar GRANT SELECT ON TABLES TO foo.
All tables created by bar will have SELECT privilege for foo.
If a role is not specified, it uses the current user.

See: https://www.postgresql.org/docs/current/sql-alterdefaultprivileges.html

Currently, default privileges are not supported on the schema.
Specifying a schema like ALTER DEFAULT PRIVILEGES IN SCHEMA s will error.

WITH GRANT OPTION is ignored.
GRANT OPTION FOR is also ignored.

[cockroach-teamcity]

This change is 

[rafiss]

having trouble following the example from the release notes

Example: ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO foo
makes it such that all tables created by the user that executed the
ALTER DEFAULT PRIVILEGES command will have SELECT privilege on the table
for user foo.

are the privileges being granted to foo or to the user who ran the command?

[RichardJCai]

having trouble following the example from the release notes

Example: ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO foo
makes it such that all tables created by the user that executed the
ALTER DEFAULT PRIVILEGES command will have SELECT privilege on the table
for user foo.

are the privileges being granted to foo or to the user who ran the command?

Yeah this concept is pretty confusing. So ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO foo is equivalent to ALTER DEFAULT PRIVILEGES FOR ROLE current_user GRANT SELECT ON TABLES TO foo.

This means any tables that current_user creates in the current database will have SELECT privilege for foo.
current_user can be thought of as the creator or object_creator and foo is still the grantee.

[rafiss]

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

---

pkg/sql/alter_default_privileges.go, line 60 at r2 (raw file):

	if n.Schema != nil {
		return nil, unimplemented.New("ALTER DEFAULT PRIVILEGES IN SCHEMA",
			"not implemented")

it might be worth filing an issue for this, just so that we can track ourselves. (and use NewWithIssue)

---

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

		newPrivs.Version = OwnerVersion
	}
	// TODO (richardjcai): We also have to handle the "public role" for types.

i'm wondering if we should just create issues for all the TODOs so nothing falls through the cracks? i'll defer to you though - what do you think will be the most helpful?

---

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

// User accesses the role field.
func (u DefaultPrivilegesForRole) User() security.SQLUsername {

nit: make the receiver a pointer

---

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

// findUser looks for a specific user in the list.
// Returns (nil, false) if not found, or (obj, true) if found.
func (p DefaultPrivilegeDescriptor) GetDefaultPrivilegesForRole(

nit: make the receiver a pointer

---

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

// findUserIndex looks for a given user and returns its
// index in the User array if found. Returns -1 otherwise.
func (p DefaultPrivilegeDescriptor) findUserIndex(user security.SQLUsername) int {

nit: make the receiver a pointer

---

pkg/sql/sem/tree/alter_default_privileges.go, line 53 at r2 (raw file):

// AlterDefaultPrivilegesTargetObject represents the type of object that is
// having it's default privileges altered.
type AlterDefaultPrivilegesTargetObject int

could this be a uint32 so that we avoid the need for a ToUint32 method? (or is it more clear with the method either way?)

---

pkg/sql/sem/tree/alter_default_privileges.go, line 56 at r2 (raw file):

const (
	Tables    AlterDefaultPrivilegesTargetObject = 1

if we need the explicit number assigning, it is worth adding a comment explaining why

if we don't need it, then we can use iota here

[RichardJCai]

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

---

pkg/sql/alter_default_privileges.go, line 60 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it might be worth filing an issue for this, just so that we can track ourselves. (and use NewWithIssue)

Done. Opened #67376

---

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

Previously, rafiss (Rafi Shamim) wrote…

i'm wondering if we should just create issues for all the TODOs so nothing falls through the cracks? i'll defer to you though - what do you think will be the most helpful?

Created a couple of TODOs:
#67376
#67377
#67378
#67381

---

pkg/sql/sem/tree/alter_default_privileges.go, line 53 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could this be a uint32 so that we avoid the need for a ToUint32 method? (or is it more clear with the method either way?)

Done.

---

pkg/sql/sem/tree/alter_default_privileges.go, line 56 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

if we need the explicit number assigning, it is worth adding a comment explaining why

if we don't need it, then we can use iota here

It's written to disk so we should keep it as explicitly definied.

[rafiss]

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

---

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

Previously, RichardJCai (Richard Cai) wrote…

Created a couple of TODOs:
#67376
#67377
#67378
#67381

thanks!

---

pkg/sql/catalog/descpb/default_privilege.go, line 178 at r4 (raw file):

// Validate returns an assertion error if the default privilege descriptor is invalid.
func (p DefaultPrivilegeDescriptor) Validate() error {

nit: make this receiver a pointer

---

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

// found to have invalid privileges and the bits representing the invalid
// privileges.
func (p PrivilegeDescriptor) IsValidPrivilegesForObjectType(

hm, i noticed this one isn't a pointer too, but it looks like all the validation functions don't take pointer receivers. i wonder why

---

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

  optional string user_proto = 1 [(gogoproto.nullable) = false,
    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/security.SQLUsernameProto"];
  map<uint32, PrivilegeDescriptor> default_privileges_per_object = 2 [(gogoproto.nullable) = false];

i think you can do something like

map<uint32, PrivilegeDescriptor> default_privileges_per_object = 2 [(gogoproto.nullable) = false,
(gogoproto.castkey) = "github.com/cockroachdb/cockroach/pkg/sql/sem/tree.AlterDefaultPrivilegesTargetObject"];

and this way we can also avoid having to do the uint32(...) conversions.

(but actually if we use castkey, i'm not sure if the AlterDefaultPrivilegesTargetObject type needs to be moved somewhere else)

[rafiss]

Looks good! thanks for making those issues. I also made one more for WITH GRANT OPTION: #67410

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

[RichardJCai]

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

---

pkg/sql/catalog/descpb/default_privilege.go, line 178 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: make this receiver a pointer

I think it does make sense to make this a pointer, not sure why the other validates aren't pointers.

---

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

Previously, rafiss (Rafi Shamim) wrote…

hm, i noticed this one isn't a pointer too, but it looks like all the validation functions don't take pointer receivers. i wonder why

I think it was likely just an oversight looking at the git history.
I'll make a separate PR to change it.

---

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

Previously, rafiss (Rafi Shamim) wrote…

i think you can do something like

map<uint32, PrivilegeDescriptor> default_privileges_per_object = 2 [(gogoproto.nullable) = false,
(gogoproto.castkey) = "github.com/cockroachdb/cockroach/pkg/sql/sem/tree.AlterDefaultPrivilegesTargetObject"];

and this way we can also avoid having to do the uint32(...) conversions.

(but actually if we use castkey, i'm not sure if the AlterDefaultPrivilegesTargetObject type needs to be moved somewhere else)

Wow nice I didn't know that. Wasn't sure how to make cast work with keys.

[arulajmani]

Thanks for sprinkling a bit more commentary on the logic tests, it made revisiting them a breeze! 💯

This thing is mostly there for me, especially once you remove the raw proto accesses and replace them with interface methods/setters.

Reviewed 3 of 29 files at r4, 30 of 30 files at r7, 11 of 29 files at r8, 14 of 19 files at r9, 9 of 9 files at r10.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @rafiss, and @RichardJCai)

---

pkg/sql/alter_default_privileges.go, line 125 at r9 (raw file):

	}

	if n.dbDesc.DatabaseDesc().GetDefaultPrivileges() == nil {

With the new catalog interfaces, I think the aim is to avoid accessing methods on the raw proto directly. We should add a GetDefaultPrivileges method on the DatabaseDescriptor interface and have immutable implement it. That way the raw proto access is limited to just that implementation.

I noticed theres a few places where this PR is following the dbDesc.DatabaseDesc().DefaultPrivileges() pattern -- might be worth cleaning those up as well!

---

pkg/sql/alter_default_privileges.go, line 126 at r9 (raw file):

	if n.dbDesc.DatabaseDesc().GetDefaultPrivileges() == nil {
		n.dbDesc.DatabaseDesc().DefaultPrivileges = descpb.InitDefaultPrivilegeDescriptor()

Instead of directly modifying the underlying proto directly here, we should make a setter method on dbdesc.Mutable and use that instead.

---

pkg/sql/grant_revoke.go, line 328 at r9 (raw file):

// validateGrantees checks that all the grantees are valid users.
// users should be returned the result of GetAllRoles.

Can we change the function signature to:

func (p *planner) validateGrantees(ctx context.Context, grantees []security.SQLUsername) error

Then, this function can make its own call to GetAllRoles instead of relying on callers to pass in the output correctly?

You might need to handle some ugly special casing for the "public" role from one of the call sites.

---

pkg/sql/catalog/dbdesc/database_desc.go, line 346 at r5 (raw file):

(would be weird to anyway since we have descriptors in the past where it should be null)

Maybe I was reading it wrong, but I thought you were treating a null default privilege descriptor as an empty privilege descriptor. So making it non-nullable would've achieved the same behaviour, right?

Regardless, considering we don't need this function anymore, this discussion is kinda moot!

---

pkg/sql/catalog/descpb/default_privilege.go, line 100 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Looks like we already handle this "public role" getting USAGE privilege in inheritUsagePrivilegeFromSchema -- can we move that code in here as part of this PR?

That is slightly different and not directly applicable here. Although I don't think this is the right place to write this TODO. I don't think we need this TODO but the issue here is that the Public role should have USAGE set in the default privileges, right now descriptors are created with no entries whereas public should always have an entry with usage. There's some more nuance to it so I'd still rather tackle this in a separate PR.

The tricky part is how do we determine if the USAGE privilege is set or not. Default privileges are defined for all roles (creator) with the set of privileges users get if the creator creates an object.

My plan is to consider Public missing from the DefaultPrivilegePerRole list entirely as "default" ie, we'd copy USAGE down. If public is in the list, then we'll treat it normally.

I don't think I fully understand what's going on here, but I'll follow it up with you offline.

[rafiss]

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

---

pkg/sql/alter_default_privileges.go, line 95 at r9 (raw file):

	granteesSQLUsername := make([]security.SQLUsername, len(grantees))
	for i, grantee := range grantees {
		granteesSQLUsername[i] = security.MakeSQLUsernameFromPreNormalizedString(string(grantee))

this string isn't actually pre-normalized (as far as I can tell); it should be turned into a username with security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation) -- see #65556 (and also the PR that fixes it)

---

pkg/sql/sem/tree/alter_default_privileges.go, line 20 at r10 (raw file):

// AlterDefaultPrivileges represents an ALTER DEFAULT PRIVILEGES statement.
type AlterDefaultPrivileges struct {
	Roles []security.SQLUsername

how is this already a []SQLUsername? it seems like it should be a NameList and converted into a username later with security.MakeSQLUsernameFromUserInput(r, security.UsernameValidation). (changing would require updating sql.y too)

you should be able to confirm by writing a logic test where the roles are mixed-case, like the test in #67625

[RichardJCai]

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

---

pkg/sql/sem/tree/alter_default_privileges.go, line 20 at r10 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

how is this already a []SQLUsername? it seems like it should be a NameList and converted into a username later with security.MakeSQLUsernameFromUserInput(r, security.UsernameValidation). (changing would require updating sql.y too)

you should be able to confirm by writing a logic test where the roles are mixed-case, like the test in #67625

I was relying on role_spec_list which returns a []SQLUsername, seems like this might be buggy though. 2 other states, DROP OWNED BY and REASSIGNED OWNED BY also currently rely on this.

We should look into fixing this, I'll file a separate issue.

[RichardJCai]

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

---

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

Previously, RichardJCai (Richard Cai) wrote…

Wow nice I didn't know that. Wasn't sure how to make cast work with keys.

Done.

---

pkg/sql/sem/tree/alter_default_privileges.go, line 20 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I was relying on role_spec_list which returns a []SQLUsername, seems like this might be buggy though. 2 other states, DROP OWNED BY and REASSIGNED OWNED BY also currently rely on this.

We should look into fixing this, I'll file a separate issue.

Fixed it by using namelist for now.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, and @rafiss)

---

pkg/sql/alter_default_privileges.go, line 95 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this string isn't actually pre-normalized (as far as I can tell); it should be turned into a username with security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation) -- see #65556 (and also the PR that fixes it)

Done.

---

pkg/sql/alter_default_privileges.go, line 125 at r9 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

With the new catalog interfaces, I think the aim is to avoid accessing methods on the raw proto directly. We should add a GetDefaultPrivileges method on the DatabaseDescriptor interface and have immutable implement it. That way the raw proto access is limited to just that implementation.

I noticed theres a few places where this PR is following the dbDesc.DatabaseDesc().DefaultPrivileges() pattern -- might be worth cleaning those up as well!

Added the function definition to the interface, turns out you don't have to implement it actually, it's considered implemented in structured.pb.go

---

pkg/sql/alter_default_privileges.go, line 126 at r9 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Instead of directly modifying the underlying proto directly here, we should make a setter method on dbdesc.Mutable and use that instead.

Done. Added SetInitialDefaultPrivilegeDescriptor on dbdesc.Mutable

---

pkg/sql/grant_revoke.go, line 328 at r9 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Can we change the function signature to:

func (p *planner) validateGrantees(ctx context.Context, grantees []security.SQLUsername) error

Then, this function can make its own call to GetAllRoles instead of relying on callers to pass in the output correctly?

You might need to handle some ugly special casing for the "public" role from one of the call sites.

Changed it to

// validateRoles checks that all the roles are valid users.
// isPublicValid determines whether or not Public is a valid role.
func (p *planner) validateRoles(
	ctx context.Context, roles []security.SQLUsername, isPublicValid bool,
) error {```
```

___
*[pkg/sql/catalog/dbdesc/database_desc.go, line 346 at r5](https://reviewable.io/reviews/cockroachdb/cockroach/66785#-Mec3KNpDim9jbw34z_I:-Mf3d8jL00cxWvyF2Kaq:b6v8lu1) ([raw file](https://github.com/cockroachdb/cockroach/blob/c93d108cffdd9ee1cb852bbcd1ef8d87cd8d90b1/pkg/sql/catalog/dbdesc/database_desc.go#L346)):*
>Maybe I was reading it wrong, but I thought you were treating a null default privilege descriptor as an empty privilege descriptor. So making it non-nullable would've achieved the same behaviour, right?

Actually I'm unsure about one thing, if we set the field to non-nullable, what happens to DatabaseDescriptors created before this version, if we injected an older descriptor would it populate the field?
___
*[pkg/sql/catalog/descpb/default_privilege.go, line 100 at r5](https://reviewable.io/reviews/cockroachdb/cockroach/66785#-MekCvSk-5mj7xaPONc6:-Mf3dR6mDYP7k2hgqJyb:b-by472x) ([raw file](https://github.com/cockroachdb/cockroach/blob/c93d108cffdd9ee1cb852bbcd1ef8d87cd8d90b1/pkg/sql/catalog/descpb/default_privilege.go#L100)):*
<details><summary><i>Previously, arulajmani (Arul Ajmani) wrote…</i></summary><blockquote>

I don't think I fully understand what's going on here, but I'll follow it up with you offline. 
</blockquote></details>

The TLDR version is that the `Public` role has USAGE by default on types when they are created.
The tricky part in handling is that we don't want to create an entry in the DefaultPrivilegesPerRole map such that each Role in CRDB has an entry in the map that is just `Public`: `USAGE ON TYPES`

It's easier and more efficient to handle the absence of this entry as meaningful.


<!-- Sent from Reviewable.io -->

[arulajmani]

Reviewed 14 of 16 files at r11.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, @rafiss, and @RichardJCai)

---

pkg/sql/alter_default_privileges.go, line 94 at r11 (raw file):

	}

	granteesSQLUsername := make([]security.SQLUsername, len(grantees))

nit: s/granteesSQLUsername/granteeSQLUsernames/g

---

pkg/sql/alter_default_privileges.go, line 131 at r11 (raw file):

	if n.dbDesc.GetDefaultPrivileges() == nil {
		n.dbDesc.SetInitialDefaultPrivilegeDescriptor(descpb.InitDefaultPrivilegeDescriptor())

nit: If this method is called SetInitialDefaultPrivilegeDescriptor, maybe it could take no arguments and make the call to descpb.InitDefaultPrivilegeDescriptor() in the function body instead? Alternatively, you could rename this to SetDefaultPrivilegeDescriptor instead.

---

pkg/sql/catalog/dbdesc/database_desc.go, line 346 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Maybe I was reading it wrong, but I thought you were treating a null default privilege descriptor as an empty privilege descriptor. So making it non-nullable would've achieved the same behaviour, right?

Actually I'm unsure about one thing, if we set the field to non-nullable, what happens to DatabaseDescriptors created before this version, if we injected an older descriptor would it populate the field?

Yeah, I think it gets populated to the "0" value, which would be an empty struct in this case?

---

pkg/sql/catalog/dbdesc/database_desc.go, line 414 at r11 (raw file):

}

// SetInitialDefaultPrivilegeDescriptor sets the initial default privilege descriptor

nit: 80 chars

---

pkg/sql/catalog/descpb/default_privilege.go, line 100 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

The TLDR version is that the Public role has USAGE by default on types when they are created.
The tricky part in handling is that we don't want to create an entry in the DefaultPrivilegesPerRole map such that each Role in CRDB has an entry in the map that is just Public: USAGE ON TYPES

It's easier and more efficient to handle the absence of this entry as meaningful.

Talked offline, your proposal sounds good to me, thanks for taking the time to explain!

---

pkg/sql/catalog/descpb/default_privilege.go, line 73 at r11 (raw file):

// the specified object with the corresponding default privileges and
// the appropriate owner (node for system, the restoring user otherwise.)
func CreatePrivilegesFromDefaultPrivileges(

nit: any reason this is a function instead of a method?

[RichardJCai]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, and @rafiss)

---

pkg/sql/alter_default_privileges.go, line 131 at r11 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: If this method is called SetInitialDefaultPrivilegeDescriptor, maybe it could take no arguments and make the call to descpb.InitDefaultPrivilegeDescriptor() in the function body instead? Alternatively, you could rename this to SetDefaultPrivilegeDescriptor instead.

Changed it to SetDefaultPrivilegeDescriptor

---

pkg/sql/catalog/dbdesc/database_desc.go, line 346 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Yeah, I think it gets populated to the "0" value, which would be an empty struct in this case?

Gotcha

---

pkg/sql/catalog/descpb/default_privilege.go, line 100 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Talked offline, your proposal sounds good to me, thanks for taking the time to explain!

Done.

---

pkg/sql/catalog/descpb/default_privilege.go, line 73 at r11 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: any reason this is a function instead of a method?

This is a function because it's possible for the DefaultPrivilegeDescriptor to be nil.

[rafiss]

exciting!

Reviewed 3 of 9 files at r1, 9 of 29 files at r8, 5 of 19 files at r9, 5 of 9 files at r10, 14 of 16 files at r11, 2 of 2 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @ajwerner, @arulajmani, and @rafiss)

[RichardJCai]

Thanks for the reviews!
bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)