Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add CONTROLCHANGEFEED role option #52869

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

solongordon
Copy link
Contributor

@solongordon solongordon commented Aug 16, 2020

Fixes #52869

Release note (sql change): Introduced a new CONTROLCHANGEFEED role
option. This grants non-admin roles the ability to create new
changefeeds, as long as they have SELECT privileges on the target table.
It can be conferred via ALTER ROLE <role> CONTROLCHANGEFEED and
revoked via ALTER ROLE <role> NOCONTROLCHANGEFEED.

@solongordon solongordon requested review from RichardJCai, a team and pbardea and removed request for a team August 16, 2020 13:50
solongordon added a commit to solongordon/cockroach that referenced this pull request Aug 16, 2020
Fixes cockroachdb#52869

Release note (sql change): Introduced a new CONTROLCHANGEFEED role
option. This grants non-admin roles the ability to create new
changefeeds, as long as they have SELECT privileges on the target table.
It can be conferred via `ALTER ROLE <role> CONTROLCHANGEFEED` and
revoked via `ALTER ROLE <role> CONTROLCHANGEFEED`.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALTER ROLE NOCONTROLCHANGEFEED to revoke instead of ALTER ROLE CONTROLCHANGEFEED in the commit message

Reviewed 1 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @pbardea, @RichardJCai, and @solongordon)


pkg/ccl/changefeedccl/changefeed_test.go, line 2002 at r1 (raw file):

			stmt = `CREATE CHANGEFEED FOR d.foo`
		}
		privErr := `user testuser does not have CONTROLCHANGEFEED privilege`

Does it make sense to turn this function into a logic test?


pkg/sql/authorization.go, line 70 at r1 (raw file):

	// HasRoleOption converts the roleoption to its SQL column name and checks if
	// the user belongs to a role where the roleprivilege has value true. Only
	// works on checking the "positive version" of the privilege. Requires a valid

I updated this in my PR but "positive version" doesn't actually make sense. I changed the function header to
// HasRoleOption converts the roleoption to it's SQL column name and
// checks if the user belongs to a role where the roleprivilege has value true.
// Requires a valid transaction to be open.
// This check should be done on the version of the privilege that is stored in
// the role options table.
// Example: CREATEROLE instead of NOCREATEROLE. NOLOGIN instead of LOGIN.

but didn't add it to the AuthorizationAccessor interface so I think it'll have to be updated here as well.


pkg/sql/parser/sql.y, line 6252 at r1 (raw file):

| NOCONTROLCHANGEFEED
	{
		$$.val = tree.KVOption{Key: tree.Name($1), Value: nil}

tabs here, I also just noticed NOCREATEROLE, LOGIN and NOLOGIN are tab spaced as well.

I need to change my goland to convert tabs to spaces for the .y file format as well

Copy link
Contributor Author

@solongordon solongordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, fixed the release note.

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


pkg/ccl/changefeedccl/changefeed_test.go, line 2002 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Does it make sense to turn this function into a logic test?

That seems like a reasonable improvement. Done.


pkg/sql/authorization.go, line 70 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I updated this in my PR but "positive version" doesn't actually make sense. I changed the function header to
// HasRoleOption converts the roleoption to it's SQL column name and
// checks if the user belongs to a role where the roleprivilege has value true.
// Requires a valid transaction to be open.
// This check should be done on the version of the privilege that is stored in
// the role options table.
// Example: CREATEROLE instead of NOCREATEROLE. NOLOGIN instead of LOGIN.

but didn't add it to the AuthorizationAccessor interface so I think it'll have to be updated here as well.

Yeah, I'll fix this once I rebase on your CONTROLJOB PR.


pkg/sql/parser/sql.y, line 6252 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

tabs here, I also just noticed NOCREATEROLE, LOGIN and NOLOGIN are tab spaced as well.

I need to change my goland to convert tabs to spaces for the .y file format as well

Ack, you infected me! I'll do a find/replace on this whole part of the file.

Copy link
Contributor

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

@pbardea pbardea removed their request for review August 20, 2020 20:15
@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 21, 2020

Merge conflict.

Fixes cockroachdb#52869

Release note (sql change): Introduced a new CONTROLCHANGEFEED role
option. This grants non-admin roles the ability to create new
changefeeds, as long as they have SELECT privileges on the target table.
It can be conferred via `ALTER ROLE <role> CONTROLCHANGEFEED` and
revoked via `ALTER ROLE <role> NOCONTROLCHANGEFEED`.
@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 22, 2020

Build failed:

@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build failed:

@solongordon
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Aug 23, 2020
52869: sql: add CONTROLCHANGEFEED role option r=solongordon a=solongordon

Fixes #52869

Release note (sql change): Introduced a new CONTROLCHANGEFEED role
option. This grants non-admin roles the ability to create new
changefeeds, as long as they have SELECT privileges on the target table.
It can be conferred via `ALTER ROLE <role> CONTROLCHANGEFEED` and
revoked via `ALTER ROLE <role> NOCONTROLCHANGEFEED`.

Co-authored-by: Solon Gordon <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build failed:

@solongordon
Copy link
Contributor Author

A different test fails every time... 😞

bors r+

craig bot pushed a commit that referenced this pull request Aug 23, 2020
52869: sql: add CONTROLCHANGEFEED role option r=solongordon a=solongordon

Fixes #52869

Release note (sql change): Introduced a new CONTROLCHANGEFEED role
option. This grants non-admin roles the ability to create new
changefeeds, as long as they have SELECT privileges on the target table.
It can be conferred via `ALTER ROLE <role> CONTROLCHANGEFEED` and
revoked via `ALTER ROLE <role> NOCONTROLCHANGEFEED`.

Co-authored-by: Solon Gordon <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build failed:

@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build failed:

@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build failed:

@solongordon
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2020

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants