sql: support alter table owner to command #52659

RichardJCai · 2020-08-12T01:11:27Z

Release note (sql change): Support ALTER TABLE OWNER TO
The command changes the owner of a table.

To use the conditions, the following conditions must be met:
The user executing the command must be the explicit owner of the table.
(Not just have membership of the role who owns the table.)
The user executing the command must be a member of the new owner role.
The new owner role must have CREATE on the schema the table belongs to.

Fixes #52020

cockroach-teamcity · 2020-08-12T01:11:34Z

This change is

rohany

Going to just cc @knz for a peek on permissions related PRs.

pkg/sql/alter_table.go

solongordon

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

pkg/sql/alter_table.go, line 742 at r2 (raw file):

				return err
			}
			descriptorChanged = changed

I think you want descriptorChanged = descriptorChanged || changed here. We don't want to flip it from true to false.

Actually it doesn't look like you're the first one to make this mistake 🙈. I'll open a separate PR to fix some of the other cases.

pkg/sql/alter_table.go, line 1170 at r2 (raw file):

	}
	if !roleExists {
		return false, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", newOwner)

Nit: Can you put the role name in quotes like postgres does?

pkg/sql/alter_table.go, line 1174 at r2 (raw file):

	// The user explicitly has to be owner to change the owner.
	if ok := IsOwner(desc, p.User()); !ok {

I don't think this is the case. It seems like you just need to have ownership, which you already check for at the beginning of ALTER TABLE. I tried it out in the Postgres and it seems fine to just be a member of the owning role.

It would be weird if you had to be the explicit owner because then if the table were owned by a role, only admins would be able to change the owner.

(Which makes me realize that this check also prevents admins from changing the owner!)

pkg/sql/alter_table.go, line 1189 at r2 (raw file):

	if _, ok := memberOf[newOwner]; !ok {
		return false, pgerror.Newf(
			pgcode.InsufficientPrivilege, "must be member of role %s", newOwner)

Nit: Can you put the role name in quotes like postgres does?

pkg/sql/alter_table.go, line 1194 at r2 (raw file):

	// Ensure the new owner has CREATE privilege on the table's schema.
	// We do not have to check for the public schema.
	if err := p.canCreateOnSchema(ctx, desc.GetParentSchemaID(), newOwner); err != nil {

Does this handle admins who don't have explicit CREATE privileges? Let's make sure to have a test for that.

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

		return nil
	case sqlbase.SchemaTemporary, sqlbase.SchemaVirtual:
		return errors.Newf("cannot CREATE on schema with id %d", resolvedSchema.ID)

Can we use a name rather than an ID here?

pkg/sql/catalog/resolver/resolver.go, line 278 at r2 (raw file):

// ResolveSchemaDescByID resolves a schema based on it's ID.
func ResolveSchemaDescByID(

@rohany Can you confirm this function looks good and isn't duplicating logic from elsewhere?

pkg/sql/catalog/resolver/resolver.go, line 288 at r2 (raw file):

	}

	// We have already considered public and virtual schemas don't have ids.

Where did you consider virtual schemas?

pkg/sql/logictest/testdata/logic_test/alter_table_owner, line 8 at r2 (raw file):

ALTER TABLE t OWNER TO fake_user

# Ensure user the current user is a member of the role we're setting to.

Nit: "user the current user"

pkg/sql/logictest/testdata/logic_test/alter_table_owner, line 14 at r2 (raw file):

user testuser

# Ensure the user has to be an owner alter the owner.

Nit: "an owner alter the owner"

RichardJCai

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

pkg/sql/alter_table.go, line 1181 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

[nit]: comment longer than 80 chars

Done.

pkg/sql/alter_table.go, line 1194 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Updated.

Done.

pkg/sql/alter_table.go, line 742 at r2 (raw file):

Previously, solongordon (Solon) wrote…

I think you want descriptorChanged = descriptorChanged || changed here. We don't want to flip it from true to false.

Actually it doesn't look like you're the first one to make this mistake 🙈. I'll open a separate PR to fix some of the other cases.

Done.

pkg/sql/alter_table.go, line 1174 at r2 (raw file):

Previously, solongordon (Solon) wrote…

I don't think this is the case. It seems like you just need to have ownership, which you already check for at the beginning of ALTER TABLE. I tried it out in the Postgres and it seems fine to just be a member of the owning role.

It would be weird if you had to be the explicit owner because then if the table were owned by a role, only admins would be able to change the owner.

(Which makes me realize that this check also prevents admins from changing the owner!)

Ah I just double checked and this is the case, I thought I had a case where it being just part of an owner role didn't work.

pkg/sql/alter_table.go, line 1194 at r2 (raw file):

I added the logic but this check should never happen for virtual schemas, I'm wondering if it should be a panic instead.
It should since it uses the regular CheckPrivilege function but I added a test as well

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

Previously, solongordon (Solon) wrote…

Can we use a name rather than an ID here?

I'm actually updating this with an assertion error - a virtual schema should not be found here since they don't have ids.
@rohany just wanna double check that what I said makes sense.

pkg/sql/catalog/resolver/resolver.go, line 288 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Where did you consider virtual schemas?

I'm actually a bit confused about this case. I thought virtual schemas can't have ids, but looking at resolver.go staticSchemaIDMap there is a mapping from some schema ids to "virtual schemas"

I added the logic but this check should never happen for virtual schemas, I'm wondering if it should be a panic/err instead.

solongordon

pending another look from @rohany.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz, @RichardJCai, and @rohany)

pkg/sql/catalog/resolver/resolver.go, line 288 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I'm actually a bit confused about this case. I thought virtual schemas can't have ids, but looking at resolver.go staticSchemaIDMap there is a mapping from some schema ids to "virtual schemas"

I added the logic but this check should never happen for virtual schemas, I'm wondering if it should be a panic/err instead.

I'm not sure about this either, @rohany?

pkg/sql/alter_table.go

pkg/sql/authorization.go

pkg/sql/catalog/resolver/resolver.go

rohany · 2020-08-16T16:05:33Z

pkg/sql/catalog/resolver/resolver.go

+	// TODO(richardjcai): We should store temp schema IDs when we create it in
+	// the session data.
+	// For now assume a nil descriptor corresponds to a temp table.
+	if desc == nil {


I'd rather just not handle the temporary schema case here than assume nil == temp. Leaving the TODO is fine however, but assign it to SQLSchema

pkg/sql/catalog/resolver/resolver.go

Release note (sql change): Support ALTER TABLE <table> OWNER TO <owner> The command changes the owner of a table. To use the conditions, the following conditions must be met: The user executing the command must be the owner of the table, or the member of the owner role. The user executing the command must be a member of the new owner role. The new owner role must have CREATE on the schema the table belongs to.

RichardJCai · 2020-08-17T22:16:34Z

Thanks for the reviews!

RichardJCai · 2020-08-17T22:16:38Z

bors r+

craig · 2020-08-17T22:32:20Z

Build succeeded:

Compile Build (Cockroach)

RichardJCai requested review from rohany and a team August 12, 2020 01:11

rohany suggested changes Aug 12, 2020

View reviewed changes

pkg/sql/alter_table.go Outdated Show resolved Hide resolved

pkg/sql/alter_table.go Outdated Show resolved Hide resolved

RichardJCai force-pushed the alter_table_owner branch from e0a63e6 to 68fc606 Compare August 12, 2020 15:41

RichardJCai requested review from a team and dt and removed request for a team August 12, 2020 15:41

RichardJCai force-pushed the alter_table_owner branch 2 times, most recently from 628d6c6 to a959e19 Compare August 12, 2020 15:57

dt removed their request for review August 12, 2020 17:41

RichardJCai force-pushed the alter_table_owner branch 2 times, most recently from da34f37 to c93a6f7 Compare August 12, 2020 21:49

RichardJCai requested a review from knz August 12, 2020 22:03

RichardJCai force-pushed the alter_table_owner branch from c93a6f7 to ecba9b9 Compare August 13, 2020 16:24

solongordon suggested changes Aug 14, 2020

View reviewed changes

RichardJCai force-pushed the alter_table_owner branch from ecba9b9 to bc4b75f Compare August 14, 2020 22:19

RichardJCai commented Aug 14, 2020

View reviewed changes

solongordon approved these changes Aug 16, 2020

View reviewed changes

rohany suggested changes Aug 16, 2020

View reviewed changes

RichardJCai force-pushed the alter_table_owner branch from bc4b75f to 43e9df6 Compare August 17, 2020 15:41

rohany suggested changes Aug 17, 2020

View reviewed changes

pkg/sql/catalog/resolver/resolver.go Show resolved Hide resolved

pkg/sql/catalog/resolver/resolver.go Show resolved Hide resolved

pkg/sql/catalog/resolver/resolver.go Outdated Show resolved Hide resolved

RichardJCai force-pushed the alter_table_owner branch from 43e9df6 to 082db3d Compare August 17, 2020 20:37

RichardJCai force-pushed the alter_table_owner branch from 082db3d to 5202e58 Compare August 17, 2020 20:38

rohany approved these changes Aug 17, 2020

View reviewed changes

craig bot merged commit 6c0dc11 into cockroachdb:master Aug 17, 2020

jseldess mentioned this pull request Sep 15, 2020

sql: support alter table owner to command cockroachdb/docs#8300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support alter table owner to command #52659

sql: support alter table owner to command #52659

RichardJCai commented Aug 12, 2020 •

edited

Loading

cockroach-teamcity commented Aug 12, 2020

rohany left a comment

solongordon left a comment

RichardJCai left a comment

solongordon left a comment

rohany Aug 16, 2020

RichardJCai commented Aug 17, 2020

RichardJCai commented Aug 17, 2020

craig bot commented Aug 17, 2020

sql: support alter table owner to command #52659

sql: support alter table owner to command #52659

Conversation

RichardJCai commented Aug 12, 2020 • edited Loading

cockroach-teamcity commented Aug 12, 2020

rohany left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

rohany Aug 16, 2020

Choose a reason for hiding this comment

RichardJCai commented Aug 17, 2020

RichardJCai commented Aug 17, 2020

craig bot commented Aug 17, 2020

RichardJCai commented Aug 12, 2020 •

edited

Loading