sql: Add ownership concept for objects

[RichardJCai]

Added ownership when creating objects. Owners have ALL privilege on the object.

Currently, ownership cannot be changed, we will need to implement the
ALTER OWNER commands for all objects.

The privileges CREATE/DROP currently exist to alleviate missing privileges
from the lack of ownership, this PR does affect CREATE/DROP privileges.

Also added testuser2 certs to allow using testuser2 in logictests to test
inheritance between multiple roles.

Release note (sql change): Added "ownership" concept objects.
Objects must have an owner, all objects that do not have owners currently
will have admin set as the default owner except system objects.
System objects without owners will have node as their owner.
By default, owners are the creator of the object. Owners have all privileges
to the objects they own. Similarly, any roles that are members of the owner
role also have all privileges on the object.

[cockroach-teamcity]

This change is 

[RichardJCai]

This is ready for review

[knz]

Ok this is very good work and a good step forward.

Code-wise this mostly looks good but needs some polish, see below.

There's a major UX problem though: as soon as a user owns some objects they cannot be DROPped until their objects have been dropped to.

We discussed that already: you can either introduce the same mechanism as in pg to re-own an object to a different role/user; or add a CASCADE clause on DROP ROLE to also delete the owned objects. Or both.

Reviewed 52 of 61 files at r1, 9 of 9 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy, @RichardJCai, and @solongordon)

---

pkg/ccl/importccl/read_import_pgdump.go, line 246 at r2 (raw file):

					id,
					hlc.Timestamp{WallTime: walltime},
					sqlbase.NewDefaultPrivilegeDescriptor(params.SessionData().User),

in the mysql case, you're checking if params.SessionData() is available.
Why is this not happening here? (Question)

---

pkg/security/securitytest/test_certs/client.testuser2.crt, line 1 at r2 (raw file):

-----BEGIN CERTIFICATE-----

Unless you have a particular reason not to, I would revert all the changes to this directory except for the addition of the testuser2 credentials and the edit to the readme file. I don't see a reason to replace every other test credential in this PR.

---

pkg/sql/authorization.go, line 94 at r2 (raw file):

	// privileges.
	// TODO(richardjcai): Add checks for if the user owns the parent of the object.
	// Ie, being the owner of a database gives access to all tables in the db.

Arguably, this TODO belongs as a comment on the isOwner function, not here.

---

pkg/sql/authorization.go, line 95 at r2 (raw file):

	// TODO(richardjcai): Add checks for if the user owns the parent of the object.
	// Ie, being the owner of a database gives access to all tables in the db.
	// Issue 51931.

nit: add the full issue URL, or write #51931 (when searching for issue references we usually always include the # sign or URL prefix)

---

pkg/sql/authorization.go, line 112 at r2 (raw file):

	// Expand role memberships.
	memberOf, err := p.MemberOfWithAdminOption(ctx, user)

This call and the loop below seem redundant with the new HasOwnership function below. IS there a way to factor them?

---

pkg/sql/create_schema.go, line 112 at r2 (raw file):

	}

	// Inherit the parent privileges.

woah, this is quite a major change.

1. can you explain the rationale in the commit message - what is this and how was it motivated? (It seems this is a fix for consistency with what is done already for tables sequences etc. If you think this is a bug fix, there may be an issue for it already.
2. add a call out in the release notes. If a bug fix, explain since when it was present and an example symptom.

---

pkg/sql/create_sequence.go, line 93 at r2 (raw file):

	// Inherit permissions from the database descriptor.
	privs := dbDesc.GetPrivileges()
	privs.SetOwner(params.SessionData().User)

Add the same node exception here as for tables.

Better yet: factor this code in a single function called from all the places.

---

pkg/sql/create_table.go, line 272 at r2 (raw file):

	privs := n.dbDesc.GetPrivileges()
	privs.SetOwner(params.SessionData().User)

I'd move these two new lines in the else branch of the if below.

---

pkg/sql/create_view.go, line 119 at r2 (raw file):

	// Inherit permissions from the database descriptor.
	privs := n.dbDesc.GetPrivileges()
	privs.SetOwner(params.SessionData().User)

Add the same node exception here as for tables.

Better yet: factor this code in a single function called from all the places.

---

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

			objectsMsg.WriteString(fmt.Sprintf("\nowner of %s %s", obj.ObjectType, obj.ObjectName))
		}
		return pgerror.Newf(pgcode.Grouping,

the pgcode here seems incorrect.

(check what pg uses in the same case)

---

pkg/sql/sqlbase/validate_test.go, line 115 at r2 (raw file):

					"perform validation. Need to know which version the descriptor is " +
					"in order to determine if an empty field is valid or not. " +
					"TODO(richardjcai): Add this field after implementing 51930."},

Is this disclaimer still needed?

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @miretskiy, and @solongordon)

---

pkg/ccl/importccl/read_import_pgdump.go, line 246 at r2 (raw file):

Previously, knz (kena) wrote…

in the mysql case, you're checking if params.SessionData() is available.
Why is this not happening here? (Question)

I think this should happen here as well, updated.

---

pkg/security/securitytest/test_certs/client.testuser2.crt, line 1 at r2 (raw file):

Previously, knz (kena) wrote…

Unless you have a particular reason not to, I would revert all the changes to this directory except for the addition of the testuser2 credentials and the edit to the readme file. I don't see a reason to replace every other test credential in this PR.

I was lazy and regenerated everything by running regenerate.sh, will revert the other changes.

---

pkg/sql/authorization.go, line 94 at r2 (raw file):

Previously, knz (kena) wrote…

Arguably, this TODO belongs as a comment on the isOwner function, not here.

Updated

---

pkg/sql/authorization.go, line 95 at r2 (raw file):

Previously, knz (kena) wrote…

nit: add the full issue URL, or write #51931 (when searching for issue references we usually always include the # sign or URL prefix)

Fixed

---

pkg/sql/authorization.go, line 112 at r2 (raw file):

Previously, knz (kena) wrote…

This call and the loop below seem redundant with the new HasOwnership function below. IS there a way to factor them?

I agree, I think it does seem a little redundant. I added the HasOwnership function separately since it seems that in some cases, we only want to check for ownership whereas CheckPrivilege checks for a privilege or ownership.

Example is RenameDatabase, we check for ownership or drop + admin, but checking for a privilege includes checking for ownership.

We can add the HasOwnership call inside the CheckPrivilege call and possibly make HasOwnership take in the roleMembership map to avoid expanding it again, although I'm not sure how much nicer/clearer this is, I don't mind the current duplication that much. What are your thoughts?

---

pkg/sql/create_schema.go, line 112 at r2 (raw file):

Previously, knz (kena) wrote…

woah, this is quite a major change.

1. can you explain the rationale in the commit message - what is this and how was it motivated? (It seems this is a fix for consistency with what is done already for tables sequences etc. If you think this is a bug fix, there may be an issue for it already.
2. add a call out in the release notes. If a bug fix, explain since when it was present and an example symptom.

This was already the behaviour, I just moved the call and comment outside of the function below. If you compare to revision 1, the logic is still the same except it sets the owner.

---

pkg/sql/create_sequence.go, line 93 at r2 (raw file):

Previously, knz (kena) wrote…

Add the same node exception here as for tables.

Better yet: factor this code in a single function called from all the places.

Done.

---

pkg/sql/create_table.go, line 272 at r2 (raw file):

Previously, knz (kena) wrote…

I'd move these two new lines in the else branch of the if below.

Moved this to its own function, I have a return in the if clause and no else clause in the createInheritedPrivilegesFromDBDesc function, let me know what you think

---

pkg/sql/create_view.go, line 119 at r2 (raw file):

Previously, knz (kena) wrote…

Add the same node exception here as for tables.

Better yet: factor this code in a single function called from all the places.

Done.

---

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

Previously, knz (kena) wrote…

the pgcode here seems incorrect.

(check what pg uses in the same case)

Fixed, think I read the pgcode wrong in the first pass

---

pkg/sql/sqlbase/validate_test.go, line 115 at r2 (raw file):

Previously, knz (kena) wrote…

Is this disclaimer still needed?

I think it is fine to leave until we implement #51930

[knz]

Reviewed 23 of 23 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy, @RichardJCai, and @solongordon)

---

pkg/sql/authorization.go, line 112 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I agree, I think it does seem a little redundant. I added the HasOwnership function separately since it seems that in some cases, we only want to check for ownership whereas CheckPrivilege checks for a privilege or ownership.

Example is RenameDatabase, we check for ownership or drop + admin, but checking for a privilege includes checking for ownership.

We can add the HasOwnership call inside the CheckPrivilege call and possibly make HasOwnership take in the roleMembership map to avoid expanding it again, although I'm not sure how much nicer/clearer this is, I don't mind the current duplication that much. What are your thoughts?

Here's my thought:

// FIXME: add the missing ctx args
func (p * planner) checkRolePredicate(user string, predicate func(role string) (bool, error)) (bool, error) {
       if ok, err := predicate(user); ok || err != nil {
         return ok, err
       }
       memberOf, err := p.MemberOfWithAdminOption(user)
	if err != nil {
		return false, err
	}
	for role := range memberOf {
          if ok, err := predicate(user); ok || err != nil {
             return ok, err
          }
	}
       return false, nil
}

Then here:

hasPriv, err := p.checkRolePredicate(sessionUser, func(role string) (bool, error) {
      if isOwner(descriptor, role) { return true, nil }
      if privs.CheckPrivilege(role, privilege) { return true, nil }
})
if err != nil {
    return err
}
if !hasPriv {
   return pgerror.Newf(pgcode.InsufficientPRivilege)
}

And separately in HasOwnership below:

func (p) HasOwnership(...) (bool, error) {
   return p.checkRolePredicate(..., func(...) ... {
       return isOwner(descriptor, role)
   })
}

🎉

---

pkg/sql/sqlbase/validate_test.go, line 115 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think it is fine to leave until we implement #51930

I don't see where in the code this version is used, hence my question. Can you point me to it?

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @miretskiy, and @solongordon)

---

pkg/sql/authorization.go, line 112 at r2 (raw file):

// FIXME: add the missing ctx args
func (p * planner) checkRolePredicate(user string, predicate func(role string) (bool, error)) (bool, error) {

Quoted 4 lines of code…

   if ok, err := predicate(user); ok || err != nil {
     return ok, err
   }
   memberOf, err := p.MemberOfWithAdminOption(user)

> if err != nil { > return false, err > } > for role := range memberOf { > if ok, err := predicate(user); ok || err != nil { > return ok, err > } > } > return false, nil > }

Good idea, I like this.

---

pkg/sql/sqlbase/validate_test.go, line 115 at r2 (raw file):

Previously, knz (kena) wrote…

I don't see where in the code this version is used, hence my question. Can you point me to it?

Oh, you're right this should be under PrivilegeDescriptor not TableDescriptor.
There doesn't seem to be a PrivilegeDescriptor validation case here. I'll look into adding it.

[solongordon]

with a few minor comments. I'm also going to circle back and take a closer look at your logic tests.

Reviewed 1 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz, @miretskiy, and @RichardJCai)

---

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

		fnl := tree.NewFmtCtx(tree.FmtSimple)
		defer fnl.Close()
		for i, name := range names {

I think it would be preferable to do what Postgres does here and only print out the first user which cannot be dropped along with what objects they own. Otherwise it's confusing as to who owns what.

---

pkg/sql/logictest/testdata/logic_test/owner, line 28 at r4 (raw file):

DROP TABLE t

user testuser

redundant?

---

pkg/sql/sqlbase/privilege.proto, line 30 at r4 (raw file):

  option (gogoproto.equal) = true;
  repeated UserPrivileges users = 1 [(gogoproto.nullable) = false];
  optional string owner = 41 [(gogoproto.nullable) = false];

Minor: Might as well this 2 rather than 41.

[RichardJCai]

Changed the unit test from pgwire_test.go into a logic test in bytes. The unit test was really annoying to update, it's now a logic test in bytes under subtest Regression_4312

The original issue was #4312

{"SELECT descriptor FROM system.descriptor WHERE descriptor != $1 LIMIT 1", []preparedQueryTest{
	baseTest.SetArgs([]byte("abc")).Results([]byte("\x12%\n\x06system\x10\x01\x1a\x15\n\t\n\x05admin\x100\n\b\n\x04root\x100\"\x00(\x01")),
}},

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @miretskiy, and @solongordon)

---

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

Previously, solongordon (Solon) wrote…

I think it would be preferable to do what Postgres does here and only print out the first user which cannot be dropped along with what objects they own. Otherwise it's confusing as to who owns what.

Fixed

---

pkg/sql/logictest/testdata/logic_test/owner, line 28 at r4 (raw file):

Previously, solongordon (Solon) wrote…

redundant?

Removed

---

pkg/sql/sqlbase/privilege.proto, line 30 at r4 (raw file):

Previously, solongordon (Solon) wrote…

Minor: Might as well this 2 rather than 41.

Updated.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @miretskiy, and @solongordon)

---

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

Previously, RichardJCai (Richard Cai) wrote…

Fixed

I think we'll also want to do this for privileges as well and combine the error messages, opened an issue for it.
#52314

[solongordon]

Reviewed 1 of 24 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @miretskiy)

[RichardJCai]

Thanks for the review!

[RichardJCai]

bors r=solongordon

[knz]

The bors run was aborted because of the merge conflict. This needs to be rebased.

[RichardJCai]

bors r=solongordon

[craig]

Build succeeded:

• Compile Build (Cockroach)