sql: SHOW ZONE CONFIGURATION only if user has permissions by otan · Pull Request #42066 · cockroachdb/cockroach

otan · 2019-10-31T15:38:18Z

Previously, SHOW ZONE CONFIGURATION did not respect permissions on
tables. Patch that up by checking permissions when using this command.

Resolves the command in #40917.

Has merge conflicts with #42046; I will fix them up with whatever lands first.

Release note (bug fix): fix bug where zones would display in SHOW ZONE CONFIGURATION even though the user has no permission to view that table

cockroach-teamcity · 2019-10-31T15:38:25Z

This change is

arulajmani

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @solongordon)

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 65 at r2 (raw file):


query TT
SHOW ZONE CONFIGURATION FOR TABLE t2

I don't know that it's worth testing these queries as root. There should be other tests that check this already.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 112 at r2 (raw file):

query TT
SHOW ZONE CONFIGURATION FOR TABLE t2
----

I think it would be better if this returned an error rather than just an empty result. For instance if a user tries to SELECT from a table they don't have access to, they get a user x does not have SELECT privilege on relation t error.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 131 at r2 (raw file):

query TT
SHOW ZONE CONFIGURATION FOR DATABASE db2
----

We also have a concept of "system ranges" for which users can configure replication zones. Only admins can configure them, but I think it would make sense to let all users view them. That would be nice so anyone can see what, for instance, the default configuration is. Your code is probably already working this way, but it's worth adding a test.

See these docs for more info:
https://www.cockroachlabs.com/docs/dev/configure-zone.html#edit-the-default-replication-zone
https://www.cockroachlabs.com/docs/dev/configure-zone.html#create-a-replication-zone-for-a-system-range

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

		}
	} else if zoneSpecifier.Database != "" {
		database, err := p.LogicalSchemaAccessor().GetDatabaseDesc(

I think you can prefer p.ResolveUncachedDatabaseByName here. See checkPrivilegeForSetZoneConfig for some similar code.

otan

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

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 65 at r2 (raw file):

Previously, solongordon (Solon) wrote…

I don't know that it's worth testing these queries as root. There should be other tests that check this already.

Done.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 112 at r2 (raw file):

Previously, solongordon (Solon) wrote…

I think it would be better if this returned an error rather than just an empty result. For instance if a user tries to SELECT from a table they don't have access to, they get a user x does not have SELECT privilege on relation t error.

Done.

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal, line 131 at r2 (raw file):

Previously, solongordon (Solon) wrote…

We also have a concept of "system ranges" for which users can configure replication zones. Only admins can configure them, but I think it would make sense to let all users view them. That would be nice so anyone can see what, for instance, the default configuration is. Your code is probably already working this way, but it's worth adding a test.

See these docs for more info:
https://www.cockroachlabs.com/docs/dev/configure-zone.html#edit-the-default-replication-zone
https://www.cockroachlabs.com/docs/dev/configure-zone.html#create-a-replication-zone-for-a-system-range

Done.

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

Previously, solongordon (Solon) wrote…

I think you can prefer p.ResolveUncachedDatabaseByName here. See checkPrivilegeForSetZoneConfig for some similar code.

Done.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

pkg/sql/show_zone_config.go, line 89 at r3 (raw file):

				row, err := getShowZoneConfigRow(ctx, p, n.ZoneSpecifier)
				if err != nil {
					v.Close(ctx)

Why was this removed?

otan

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

pkg/sql/show_zone_config.go, line 89 at r3 (raw file):

Previously, solongordon (Solon) wrote…

Why was this removed?

bad copy paste!

Previously, SHOW ZONE CONFIGURATION did not respect permissions on tables. Patch that up by checking permissions when using this command. Release note (bug fix): fix bug where zones would display in `SHOW ZONE CONFIGURATION` even though the user has no permission to view that table

solongordon

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

otan · 2019-10-31T20:23:23Z

bors r+

craig · 2019-10-31T20:36:07Z

Build failed

GitHub CI (Cockroach)

otan · 2019-10-31T20:50:25Z

bors r+

41914: sqlmigrations: don't run baked-in migrations r=andreimatei a=andreimatei Before this patch, the migration runner had a mechanism by which it attempted to not run migrations that were included in the cluster bootstrap schema. That mechanism was weak. A migration could be marked as includedInBootstrap, and then the migration runner would skip it if the current node had just performed a bootstrap. However, the respective migration would still be run (aimlessly) by a 2nd node joining the cluster, or even by the same node that just skipped them, after a restart. This patch improves the mechanism such that migrations included in the bootstrap image are never run if the cluster's bootstrap version is high enough. It does so by allowing one to mark migrations with the version at which they've been introduced, and the runner will skip them if the cluster's bootstrap version is >=. This means that migrations that want to be marked in this way also need to introduce a cluster version. The benefit of this change, besides faster node startup time, removal of confusing entries from the system.event_log table and more determinism (because now some migrations are never run instead of not run on single node clusters but run on multi-node clusters), is that logic tests that hardcode expected cluster events are more easily written. Before this patch, those tests had to explicitly ignore some events that are generated by migrations (because, again, depending on the configuration, the migration would sometimes run and sometimes not). It's now the second time this happens to a change that I personally want to make, and it's very surprising and scary when it happens to you. You can see the edit in the event_log logic test for a case that I'm not correcting (there's probably others in the same file but I haven't looked). Release note: None. 41991: storage/engine: move MVCCIncrementalIterator from storageccl to engine r=petermattis a=hueypark This is prerequisites for [storage/engine: add Pebble support to ExportToSst](#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](75f1314). See #41945 Release note: None 42066: sql: SHOW ZONE CONFIGURATION only if user has permissions r=otan a=otan Previously, SHOW ZONE CONFIGURATION did not respect permissions on tables. Patch that up by checking permissions when using this command. Resolves the command in #40917. Has merge conflicts with #42046; I will fix them up with whatever lands first. Release note (bug fix): fix bug where zones would display in `SHOW ZONE CONFIGURATION` even though the user has no permission to view that table Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

craig · 2019-10-31T21:30:32Z

Build succeeded

GitHub CI (Cockroach)

otan requested review from a team and solongordon October 31, 2019 15:38

otan force-pushed the otan-real_fix_zone_config branch from d1f2f3d to 9d95a46 Compare October 31, 2019 16:00

arulajmani approved these changes Oct 31, 2019

View reviewed changes

otan force-pushed the otan-real_fix_zone_config branch 3 times, most recently from 08cfcb6 to 87ffd70 Compare October 31, 2019 18:20

solongordon suggested changes Oct 31, 2019

View reviewed changes

otan commented Oct 31, 2019

View reviewed changes

otan force-pushed the otan-real_fix_zone_config branch from 87ffd70 to 21183c3 Compare October 31, 2019 19:44

otan requested a review from solongordon October 31, 2019 19:44

solongordon suggested changes Oct 31, 2019

View reviewed changes

otan force-pushed the otan-real_fix_zone_config branch from 21183c3 to e239e1e Compare October 31, 2019 20:20

otan commented Oct 31, 2019

View reviewed changes

otan force-pushed the otan-real_fix_zone_config branch from e239e1e to 7ea800b Compare October 31, 2019 20:21

otan requested a review from solongordon October 31, 2019 20:21

solongordon approved these changes Oct 31, 2019

View reviewed changes

craig bot merged commit 7ea800b into cockroachdb:master Oct 31, 2019

This was referenced Dec 5, 2019

ccl/logictestccl: TestCCLLogic failed under stress #42098

Closed

sql: bugfixes in role membership caching #42998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: SHOW ZONE CONFIGURATION only if user has permissions#42066

sql: SHOW ZONE CONFIGURATION only if user has permissions#42066
craig[bot] merged 1 commit intocockroachdb:masterfrom
otan-cockroach:otan-real_fix_zone_config

otan commented Oct 31, 2019 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 31, 2019

Uh oh!

arulajmani left a comment

Uh oh!

solongordon left a comment

Uh oh!

otan left a comment

Uh oh!

solongordon left a comment

Uh oh!

otan left a comment

Uh oh!

solongordon left a comment

Uh oh!

otan commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Uh oh!

otan commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

otan commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 31, 2019

Uh oh!

arulajmani left a comment

Choose a reason for hiding this comment

Uh oh!

solongordon left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

solongordon left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

solongordon left a comment

Choose a reason for hiding this comment

Uh oh!

otan commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Build failed

Uh oh!

otan commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otan commented Oct 31, 2019 •

edited

Loading